NotificationsYou must be signed in to change notification settings
Fork33.3k
Star69.7k

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Closed

dgiger42 wants to merge3 commits intopython:mainfromdgiger42:optimize_condition_notify

Closed

gh-92352 optimize Condition.notify()#92364

dgiger42 wants to merge3 commits intopython:mainfromdgiger42:optimize_condition_notify

Conversation

Copy link

Contributor

dgiger42 commentedMay 6, 2022•
edited
Loading

Closes#92352

This speeds upCondition.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 version1 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.

optimize notify()

049756e

Copy link

ghost commentedMay 6, 2022•
edited by ghost
Loading

All commit authors signed the Contributor License Agreement.

Copy link

bedevere-bot commentedMay 6, 2022

Every change to Pythonrequires a NEWS entry.

Please, add it using theblurb_it Web app or theblurb command-line tool.

bedevere-bot added the awaiting review label

May 6, 2022

methane reviewed

May 6, 2022

View reviewed changes

Lib/threading.py

		ifnotself._is_owned():
		raiseRuntimeError("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 removingwaiters_to_notify,all_waiters looks bit ugly.
Please renameall_waiters to justwaiters.

blurb-itbotand others added2 commits

May 6, 2022 03:27

📜🤖 Added by blurb_it.

b950228

rename all_waiters to waiters

25d7683

hauntsaninja reviewed

May 7, 2022

View reviewed changes

Lib/threading.py

		forwaiterinwaiters_to_notify:
		num_to_notify=min(n,len(waiters))
		foriinrange(num_to_notify):
		waiter=waiters.popleft()

Copy link

Contributor

hauntsaninjaMay 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 aroundall_waiters.remove concerns me slightly, since it implies thatself._waiters can racily lose elements.

Maybe we should guard against waiters being emptyif not waiters: break or catchIndexError?

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

hauntsaninjaMay 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 longernum_to_notify we don't crash.

Copy link

ContributorAuthor

dgiger42May 8, 2022

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 lockand 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 thatself._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 inCondition.

If preferable, I could add the try-except back.

AlexWaygood added performance

Performance or resource usage

stdlibStandard Library Python modules in the Lib/ directory labels

May 7, 2022

serhiy-storchaka reviewed

May 8, 2022

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.

There is a chance to get aKeyboardInterrupt orMemoryError betweenpopleft() andrelease(). You should handle this hard-to-reproduce case.

Copy link

ContributorAuthor

dgiger42 commentedMay 8, 2022

There is a chance to get aKeyboardInterrupt orMemoryError betweenpopleft() andrelease(). You should handle this hard-to-reproduce case.

@serhiy-storchaka would switching back torelease()ing before taking thewaiter out of the deque fix that, or did the previous version of the code already have a similar issue?

Copy link

Member

serhiy-storchaka commentedMay 9, 2022

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