-
-
Notifications
You must be signed in to change notification settings - Fork 32.2k
gh-92352 optimize Condition.notify() #92364
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Every change to Python requires a NEWS entry. Please, add it using the blurb_it Web app or the blurb command-line tool. |
@@ -369,15 +369,13 @@ def notify(self, n=1): | |||
if not self._is_owned(): | |||
raise RuntimeError("cannot notify on un-acquired lock") | |||
all_waiters = self._waiters | |||
waiters_to_notify = _deque(_islice(all_waiters, n)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After removing waiters_to_notify
, all_waiters
looks bit ugly.
Please rename all_waiters
to just waiters
.
for waiter in waiters_to_notify: | ||
num_to_notify = min(n, len(waiters)) | ||
for i in range(num_to_notify): | ||
waiter = waiters.popleft() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The try-except in the old code around all_waiters.remove
concerns me slightly, since it implies that self._waiters
can racily lose elements.
Maybe we should guard against waiters being empty if not waiters: break
or catch IndexError
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think waiters must be guarded by lock.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure we need another lock (beyond the GIL) / the previous code seems fine to me. Just need to make sure if enough waiters get removed elsewhere that there are no longer num_to_notify
we don't crash.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The previous version guarded it with a lock and had the try-except. The reason I didn't use try-except was because having both seemed redundant to me. The only way I can think of that self._waiters
could racily lose elements is if a different function removed an element without the lock, which isn't done in any of the methods in Condition
.
If preferable, I could add the try-except back.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a chance to get a KeyboardInterrupt
or MemoryError
between popleft()
and release()
. You should handle this hard-to-reproduce case.
@serhiy-storchaka would switching back to |
The previous version of the code already have a similar issue, but with different manifestation. If you interrupt after releasing the lock, but before removing it from the queue, you will get a RuntimeError next time you call We should first fix an issue in the existing code. #92530 |
It seems that the proper fix of the interruption bug includes also an optimization similar to this PR. #92534 |
Closes #92352
This speeds up
Condition.notify()
. I benchmarked it with this command:./python -m timeit -s 'import queue; q = queue.Queue()' 'for i in range(10 ** 6): q.put(42)'
Here are the timings with and without this PR:
With this change,
notify()
no longer creates a new_deque
every time. I think the speedup tends to be most noticeable when there aren't any threads waiting.