NotificationsYou must be signed in to change notification settings
Fork32k
Star67.3k

gh-132551: make BytesIO object free-thread safe#132616

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

kumaraditya303 merged 13 commits intopython:mainfromtom-pytel:fix-issue-132551

May 8, 2025

Merged

gh-132551: make BytesIO object free-thread safe#132616

kumaraditya303 merged 13 commits intopython:mainfromtom-pytel:fix-issue-132551

May 8, 2025

Conversation

Copy link

Contributor

tom-pytel commentedApr 16, 2025•
edited by bedevere-appbot
Loading

Fullpyperformance suite times (can anyone can suggest better benchmark forBytesIO?):

             time diff         mean    stdevGIL    -0.01% +- 2.31%noGIL  +0.22% +- 2.93%

Changes,* functions had critical section added,- not. Atomics were added forexports field:

static PyGetSetDef bytesio_getsetlist[] = {*   {"closed",  bytesio_get_closed, NULL,    // @critical_section to avoid make `buf` atomic many placesstatic struct PyMethodDef bytesio_methods[] = {-   _IO_BYTESIO_READABLE_METHODDEF-   _IO_BYTESIO_SEEKABLE_METHODDEF-   _IO_BYTESIO_WRITABLE_METHODDEF*   _IO_BYTESIO_CLOSE_METHODDEF-   _IO_BYTESIO_FLUSH_METHODDEF-   _IO_BYTESIO_ISATTY_METHODDEF*   _IO_BYTESIO_TELL_METHODDEF      // @critical_section to avoid make `pos` atomic many places*   _IO_BYTESIO_WRITE_METHODDEF*   _IO_BYTESIO_WRITELINES_METHODDEF*   _IO_BYTESIO_READ1_METHODDEF*   _IO_BYTESIO_READINTO_METHODDEF*   _IO_BYTESIO_READLINE_METHODDEF*   _IO_BYTESIO_READLINES_METHODDEF*   _IO_BYTESIO_READ_METHODDEF*   _IO_BYTESIO_GETBUFFER_METHODDEF*   _IO_BYTESIO_GETVALUE_METHODDEF*   _IO_BYTESIO_SEEK_METHODDEF      // @critical_section to avoid make `pos` atomic many places*   _IO_BYTESIO_TRUNCATE_METHODDEF*   {"__getstate__", bytesio_getstate,  METH_NOARGS, NULL},*   {"__setstate__", bytesio_setstate,  METH_O, NULL},*   {"__sizeof__",   bytesio_sizeof,     METH_NOARGS, NULL},static PyType_Slot bytesio_slots[] = {-   {Py_tp_dealloc,   bytesio_dealloc},-   {Py_tp_traverse,  bytesio_traverse},-   {Py_tp_clear,     bytesio_clear},-   {Py_tp_iter,      PyObject_SelfIter},*   {Py_tp_iternext,  bytesio_iternext},*   {Py_tp_init,      _io_BytesIO___init__},-   {Py_tp_new,       bytesio_new},static PyType_Slot bytesiobuf_slots[] = {-   {Py_tp_dealloc, bytesiobuf_dealloc},-   {Py_tp_traverse, bytesiobuf_traverse},-   {Py_bf_getbuffer, bytesiobuf_getbuffer},-   {Py_bf_releasebuffer, bytesiobuf_releasebuffer},

Notes:

Opted for critical sections in three functions instead of makingbuf and orpos atomic everywhere.
New testtest_free_threading.test_io.TestBytesIO clean under--parallel-threads TSAN.
bytesiobuf_getbuffer doesn't need critical sections because as far as I see its only used forBytesIO inbytesio.c where its use is protected by a critical section in_io_BytesIO_getbuffer_impl (its also used for a random non-BytesIO-related test in_testcapimodule.c).

Issue:Segfault/abort from callingBytesIOunshare_buffer in threads on a free-threaded build #132551

pythongh-132551: make BytesIO object free-thread safe

a822478

bedevere-appbot added the awaiting review label

Apr 16, 2025

bedevere-appbot mentioned this pull request

Apr 16, 2025

Segfault/abort from callingBytesIOunshare_buffer in threads on a free-threaded build#132551

Closed

blurb-itbotand others added2 commits

April 16, 2025 21:03

📜🤖 Added by blurb_it.

b384a71

fix duplicate func in test

3aa68a4

Copy link

ContributorAuthor

tom-pytel commentedApr 16, 2025

Ping@ZeroIntensity,@serhiy-storchaka

ZeroIntensity added the topic-free-threading label

Apr 16, 2025

ZeroIntensity reviewed

Apr 16, 2025

View reviewed changes

Copy link

Member

ZeroIntensity left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

The implementation looks fine, but I'm worried that concurrent use ofBytesIO is an actual use-case that we need to consider.

For example, I could see someone using a logger that writes to an IO-like object from multiple threads. If they wanted to log to aBytesIO buffer (e.g., for temporarily suppressing the output), then that would scale pretty terribly with critical sections. I'm speculating, though.

Copy link

ContributorAuthor

tom-pytel commentedApr 16, 2025

For example, I could see someone using a logger that writes to an IO-like object from multiple threads. If they wanted to log to aBytesIO buffer (e.g., for temporarily suppressing the output), then that would scale pretty terribly with critical sections. I'm speculating, though.

Well, that's why I asked about a specific better benchmark to test with becausepyperformance is not specific enough to this to show a perf hit. But as for the case you mention, if someone is writing theBytesIO object from multiple threads then performance is not really an issue as they will eventually segfault.

$ ./python ../t.py 12Segmentation fault (core dumped)

t.py:

fromioimportBytesIOfromrandomimportrandintimportthreadingdefwrite(barrier,b):barrier.wait()b.write(b'0'*randint(100,1000))defcheck(funcs,*args):barrier=threading.Barrier(len(funcs))thrds= []forfuncinfuncs:thrd=threading.Thread(target=func,args=(barrier,*args))thrds.append(thrd)thrd.start()forthrdinthrds:thrd.join()if__name__=="__main__":count=0whileTrue:print(count:=count+1)check([write]*10,BytesIO())

Copy link

Member

ZeroIntensity commentedApr 16, 2025•
edited
Loading

Well, yeah, we need to fix the races. I'm just wonderinghow we should fix it. Would you mind benchmarking (with this PR) the case I brought up against different numbers of threads? I want to see how bad the contention gets (and compare it to a GILful build as well).

Copy link

ContributorAuthor

tom-pytel commentedApr 16, 2025

Well, yeah, we need to fix the races. I'm just wonderinghow we should fix it. Would you mind benchmarking (with this PR) the case I brought up against different numbers of threads? I want to see how bad the contention gets (and compare it to a GILful build as well).

Ok, will do a bunch of tests tomorrow.

Copy link

ContributorAuthor

tom-pytel commentedApr 17, 2025•
edited
Loading

EDIT: I just realized with a good bit of rewriting lock-free reads might be possible. EDIT2: Nope, see below.

TL;DR: GIL build performance is same as expected while noGIL makes multithread writes possible and otherwise seems ok.

Detail: noGIL PR has a minor hit to single-threaded read and a smaller hit to single-threaded write (cmdline timeit). Multithreaded timings are unstable (especially write, run the script and u will get 2x variation for that), but read looks slower than main while write is much faster than main with explicit locking (which is the only thing to compare it to as otherwise writes crash).

./python -m timeit -s "from io import BytesIO; b = BytesIO((b'.'*99+b'\n')*50_000)" "while b.read(100): pass"./python -m timeit -s "from io import BytesIO; b = BytesIO((b'.'*99+b'\n')*50_000)" "while b.readline(): pass"./python -m timeit -s "from io import BytesIO; b = BytesIO(); l = b'.'*99+b'\n'" "for _ in range(50_000): b.write(l)"

Times in ns, lower is better.

SINGLE MAIN THREAD '-m timeit' cmdline (above):  (per loop, read/readline = 20000000 loops, write = 100 loops)=====================================             read  readline     writeGIL main     18.3      14.4      2580GIL PR       18.2      14.2      2590noGIL main   18.8      14.7      3710noGIL PR     20.3      16.1      3860MULTITHREADING benchmark script (below):========================================read:-----cfg / #thrds     1      2      3      4      5GIL main      24.4   23.6   23.8   23.7   23.7GIL PR        23.4   23.7   24.1   23.5   24.8noGIL main    27.4   44.0   64.8    124    116noGIL PR      28.6   57.6   90.5    129    163readline:---------cfg / #thrds     1      2      3      4      5GIL main      22.4   22.3   22.8   22.2   23.7GIL PR        22.0   22.2   22.7   21.9   25.0noGIL main    25.4   46.2   85.8    198    261noGIL PR      26.1   54.7   82.8    114    152write:  (* = with threading.Lock for 'noGIL main', otherwise crash)------cfg / #thrds     1      2      3      4      5GIL main      22.6   22.2   24.6   22.4   23.0GIL PR        22.1   23.2   22.4   23.0   23.4noGIL main    42.2    286*   571*   805*  1077*noGIL PR      39.7    123    287    535    677write:  (different script (not below) to double check unstable timings, total time)------cfg / #thrds         1          2          3          4          5GIL main       1171556    2474471    3664172    4817181    6343492GIL PR         1195404    2442720    3698038    4949697    6276943noGIL main     5997412   22573890*  27660777*  43626582*  61317669*noGIL PR       2332928    6786042   19316932   38951468   31712621

Ping@ZeroIntensity, timings as you requested.

importsysfromioimportBytesIOfromthreadingimportBarrier,Lock,Threadfromtimeimporttime_nsBESTOF=5NLINES=50_000LINE=b'.'*99+b'\n'LLEN=len(LINE)LOCK=Lock()defread(barrier,out,b):barrier.wait()t0=time_ns()whileb.read(LLEN):passout[0]=time_ns()-t0defreadline(barrier,out,b):barrier.wait()t0=time_ns()whileb.readline():passout[0]=time_ns()-t0defwrite_locked(barrier,out,b):barrier.wait()t0=time_ns()for_inrange(NLINES):withLOCK:b.write(LINE)out[0]=time_ns()-t0defwrite(barrier,out,b):barrier.wait()t0=time_ns()for_inrange(NLINES):b.write(LINE)out[0]=time_ns()-t0defcheck(func,nthrds,b,is_wr):# is_wr compensates for write total size *= nthrds while read total size is fixedbarrier=Barrier(nthrds)outs= []thrds= []for_inrange(nthrds):out= [0]thrd=Thread(target=func,args=(barrier,out,b))outs.append(out)thrds.append(thrd)thrd.start()forthrdinthrds:thrd.join()returnsum(o[0]foroinouts)/ ((nthrdsifis_wrelse1)*NLINES)defcheckbest(func,nthrds,setup,is_wr):best=float('inf')for_inrange(BESTOF):best=min(best,check(func,nthrds,setup(),is_wr))returnbestif__name__=="__main__":nthrds=int((sys.argv+ ['1'])[1])print(f'read:         ',checkbest(read,nthrds,lambda:BytesIO(LINE*NLINES),False),'ns')print(f'readline:     ',checkbest(readline,nthrds,lambda:BytesIO(LINE*NLINES),False),'ns')print(f'write_locked: ',checkbest(write_locked,nthrds,lambda:BytesIO(),True),'ns')print(f'write:        ',checkbest(write,nthrds,lambda:BytesIO(),True),'ns')

serhiy-storchaka reviewed

Apr 17, 2025

View reviewed changes

Copy link

Member

serhiy-storchaka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Many functions are just wrapped in critical section. I wonder, what if add special flag, METH_FT_SYNC, and when it set, wrap the method calling code in critical section?

Copy link

ContributorAuthor

tom-pytel commentedApr 17, 2025

Many functions are just wrapped in critical section. I wonder, what if add special flag, METH_FT_SYNC, and when it set, wrap the method calling code in critical section?

If you're talking about stuff likebytesio_sizeof, I could have just handed that over to argument clinic to wrap but I guess I was being lazy. Argument clinic works well enough for the critical section wrapping I think.

Copy link

Member

serhiy-storchaka commentedApr 17, 2025

I mean that this would work also for methods that do not use Argument Clinic. Was this idea discussed before?

Copy link

ContributorAuthor

tom-pytel commentedApr 17, 2025

I mean that this would work also for methods that do not use Argument Clinic. Was this idea discussed before?

Couldn't tell you that, I'm relatively new here (: But one drawback I can see would be that it would only work when calling via py mechanism, if u called directly from C you would still have to do the critical section on your own as opposed to a clinic-generated function which would have it, no?

change SHARED_BUF() to use _PyObject_IsUniquelyReferenced()

e5cd6e9

Copy link

Member

ZeroIntensity commentedApr 17, 2025

I mean that this would work also for methods that do not use Argument Clinic. Was this idea discussed before?

Yeah, I've suggested it in the past. Apparently, it's been tried and failed before. I don't know much of the details.

ZeroIntensity reviewed

Apr 17, 2025

View reviewed changes

Modules/_io/bytesio.c OutdatedShow resolvedHide resolved

remove dummies from _lock_held functions

605c2ad

Copy link

ContributorAuthor

tom-pytel commentedApr 17, 2025

Ok, had a look and lock-free reads can almost certainly be done but they would not have the data integrity guarantees you would want from a file-like object without sync. They would work in the same way lock-free multithreaded iteration works, could get dup data and might not sync position correctly wrt writes, which is not what you expect from a "file". So scratch that idea.