Skip to content

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.

Sign up for GitHub

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

Closed
wants to merge 3 commits into from
Closed

gh-92352 optimize Condition.notify() #92364

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
049756e
optimize notify()
dgiger42 May 6, 2022
b950228
📜🤖 Added by blurb_it.
blurb-it[bot] May 6, 2022
25d7683
rename all_waiters to waiters
dgiger42 May 6, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 6 additions & 8 deletions Lib/threading.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -368,16 +368,14 @@ 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))
Copy link
Member

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.

if not waiters_to_notify:
waiters = self._waiters

if not waiters:
return
for waiter in waiters_to_notify:
num_to_notify = min(n, len(waiters))
for i in range(num_to_notify):
waiter = waiters.popleft()
Copy link
Contributor

@hauntsaninja hauntsaninja May 7, 2022 •
edited
Loading

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?

Copy link
Member

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.

Copy link
Contributor

@hauntsaninja hauntsaninja May 7, 2022 •
edited
Loading

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.

Copy link
Contributor Author

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.

waiter.release()
try:
all_waiters.remove(waiter)
except ValueError:
pass

def notify_all(self):
"""Wake up all threads waiting on this condition.
Expand Down
1 change: 1 addition & 0 deletions Misc/NEWS.d/next/Library/2022-05-06-03-27-58.gh-issue-92352.Tv7wYm.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Optimizes ``Condition.notify()`` by removing unneeded construction of a ``_deque``