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.

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

Conversation

dgiger42
Copy link
Contributor

@dgiger42 dgiger42 commented May 6, 2022

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:

1 loop, best of 5: 1.93 sec per loop  # original version
1 loop, best of 5: 1.37 sec per loop  # with 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.

@ghost
Copy link

ghost commented May 6, 2022

All commit authors signed the Contributor License Agreement.
CLA signed

@bedevere-bot
Copy link

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))
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.

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

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

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.

@AlexWaygood AlexWaygood added performance Performance or resource usage stdlib Python modules in the Lib dir labels May 7, 2022
Copy link
Member

@serhiy-storchaka 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.

There is a chance to get a KeyboardInterrupt or MemoryError between popleft() and release(). You should handle this hard-to-reproduce case.

@dgiger42
Copy link
Contributor Author

dgiger42 commented May 8, 2022

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 release()ing before taking the waiter out of the deque fix that, or did the previous version of the code already have a similar issue?

@serhiy-storchaka
Copy link
Member

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 notify(). If you interrupt after removing the lock from the queue, but before releasing the lock, the waiter thread will hang forever. In the former case you at least notice that something is wrong, in the later case it hangs silently.

We should first fix an issue in the existing code. #92530

@serhiy-storchaka
Copy link
Member

It seems that the proper fix of the interruption bug includes also an optimization similar to this PR. #92534

@dgiger42
Copy link
Contributor Author

It seems that the proper fix of the interruption bug includes also an optimization similar to this PR. #92534

Ok. I'll close this PR in favor of #92534

@dgiger42 dgiger42 closed this May 15, 2022
@dgiger42 dgiger42 deleted the optimize_condition_notify branch October 4, 2022 23:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting review performance Performance or resource usage stdlib Python modules in the Lib dir
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Speed up Condition.notify()
6 participants