gh-113884: Make queue.SimpleQueue thread-safe in --disable-gil builds #114161
Conversation
Misc/NEWS.d/next/Core and Builtins/2024-01-17-00-52-57.gh-issue-113884.CvEjUE.rst
Outdated
Show resolved
Hide resolved
|
Could you split it up so the ring buffer refactoring is done first, then a follow-up PR to make it thread-safe? |
@erlend-aasland I think it makes the most sense for these commits to be merged together as part of a single PR. The commits in this PR are structured in a way that they can be reviewed independently. How about we start by reviewing the first commit, which is the ring buffer refactoring, then go from there? |
|
Tagging @colesbury |
AFAICS, the ring buffer refactoring makes for a nice stand-alone PR. It will result in a cleaner Git history, and it will make bisecting easier when hunting for bugs. |
I disagree but will split the PR in the interest of moving things forward. Do you have any suggestions for the appropriate "size" of a PR? This will help me avoid having to split PRs in the future. I've been operating under the assumption that a PR should roughly correspond to a single, self-contained "feature" that stands on its own. I don't think the refactoring fits this model, as it wouldn't make sense to do without the other changes in the PR. |
|
Split the ring buffer refactoring out into #114259. Putting this in draft until that PR is merged. |
We normally do not mix refactorings and features. Most refactorings are done in order to make the code more readable, and/or structure the code in a more maintainable way. Those are qualities that stand on their own, and can justify a single (or multiple) PRs. |
|
As for PR size, I think it is good to try and keep the diff as small as possible (we don't like churn), and keep the number of changed lines within a couple of hundred lines. (Of course, generated files do not count.) |
Methods on SimpleQueue are protected with the per-object lock.
…e-113884.CvEjUE.rst
We're not using `PyThread_acquire_lock_timed`.
mpage
force-pushed
the
gh-113884-ft-simplequeue
branch
from
cdf0917 to
d3b5547
Compare
|
I've rebased this against main after the ring buffer refactoring was merged. @colesbury and @erlend-aasland would you please have another look? |
There was a problem hiding this comment.
Thanks. The parking lot API still feels little bit uncomfortable for me, but AFAICS, this looks good. Perhaps a comment explaining how the hand-off data structure relates to the parking lot API could be beneficial for future readers of the code. I also find the maybe_unparked_thread oddly named. OTOH, I don't have a better suggestion :)
Misc/NEWS.d/next/Core and Builtins/2024-01-17-00-52-57.gh-issue-113884.CvEjUE.rst
Outdated
Show resolved
Hide resolved
|
@mpage, I applied my minor suggestions. Let me know what you think of #114161 (comment). |
Rename parking lot callback to better reflect what it does. More precise type for whether handoff occurred.
|
@erlend-aasland - I think I've addressed all the comments now, can you take another look? |
…isabled (python#114161) * use the ParkingLot API to manage waiting threads * use Argument Clinic's critical section directive to protect queue methods * remove unnecessary overflow check Co-authored-by: Erlend E. Aasland <erlend.aasland@protonmail.com>
| @@ -164,7 +167,7 @@ RingBuf_Put(RingBuf *buf, PyObject *item) | |||
| return -1; | |||
There was a problem hiding this comment.
You need to decref item here isn't it?
…isabled (python#114161) * use the ParkingLot API to manage waiting threads * use Argument Clinic's critical section directive to protect queue methods * remove unnecessary overflow check Co-authored-by: Erlend E. Aasland <erlend.aasland@protonmail.com>
Make
queue.SimpleQueuethread-safe when the GIL is disabled. Queue state is protected by the per-object lock; thread suspension now uses the low-levelPyParkingLotabstraction.--disable-gilbuilds #113884