Skip to content
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

gh-112050: Make collections.deque thread-safe in free-threaded builds #113830

Merged
merged 58 commits into from
Feb 15, 2024

Conversation

mpage
Copy link
Contributor

@mpage mpage commented Jan 8, 2024

This uses critical sections to make deque methods that operate on mutable state thread-safe when the GIL is disabled. This is mostly accomplished by using the @critical_section clinic directive, though there are a few places where this was not possible and critical sections had to be manually acquired/released.

Convert most methods on collections.deque to use clinic. This will
allow us to use clinic's `@critical_section` directive when making
deques thread-safe for `--gil-disabled`, simplifying the implementation.
Clear is used only by the GC; no other threads are running.
@erlend-aasland
Copy link
Contributor

I'll get some benchmark numbers in a bit.

Great; let's wait with landing this until we've got those.

@mpage
Copy link
Contributor Author

mpage commented Feb 2, 2024

@erlend-aasland Here are some benchmark numbers using the default -O3. Let me know if you'd like me to run others. The only substantive change in this PR for default builds is to len, where we're now using an atomic load. I would expect that to be slower, though the benchmark numbers don't appear to confirm that assumption. If we're concerned about a slowdown there we could consider using an ifdef to limit the atomic load to only free-threaded builds.

len

I would expect a slowdown due to the use of atomics, but the data doesn't appear to support that.

This PR:

> ./python -m timeit -s "import collections; d = collections.deque(list(range(100)))" "len(d)"
10000000 loops, best of 5: 21.2 nsec per loop

main:

> ./python -m timeit -s "import collections; d = collections.deque(list(range(100)))" "len(d)"
10000000 loops, best of 5: 20.8 nsec per loop
__init__

This should be representative of methods that were annotated with the @critical_section directive. I wouldn't expect any performance change here; the generated acquire/release pair should be a no-op.

This PR:

> ./python -m timeit -s "import collections; L = list(range(100))" "collections.deque(L, 50)"
500000 loops, best of 5: 733 nsec per loop

> ./python -m timeit -s "import collections; L = list(range(100))" "collections.deque(L)"
500000 loops, best of 5: 769 nsec per loop

Main:

> ./python -m timeit -s "import collections; L = list(range(100))" "collections.deque(L, 50)"
500000 loops, best of 5: 731 nsec per loop

> ./python -m timeit -s "import collections; L = list(range(100))" "collections.deque(L)"
500000 loops, best of 5: 746 nsec per loop
contains

This should be representative of methods that had critical sections added manually. In the case of contains, that split the function into two. I wouldn't expect any performance change here in default builds. The compiler should still be able to fully optimize the refactored code.

This PR:

> ./python -m timeit -s "import collections; L = list(range(100))" "50 in L"
500000 loops, best of 5: 512 nsec per loop
> ./python -m timeit -s "import collections; L = list(range(100))" "1 in L"
10000000 loops, best of 5: 29.6 nsec per loop
> ./python -m timeit -s "import collections; L = list(range(100))" "100 in L"
500000 loops, best of 5: 1.02 usec per loop

main:

> ./python -m timeit -s "import collections; L = list(range(100))" "50 in L"
500000 loops, best of 5: 520 nsec per loop
> ./python -m timeit -s "import collections; L = list(range(100))" "1 in L"
10000000 loops, best of 5: 30.2 nsec per loop
> ./python -m timeit -s "import collections; L = list(range(100))" "100 in L"
200000 loops, best of 5: 1 usec per loop
>

@mpage
Copy link
Contributor Author

mpage commented Feb 14, 2024

@erlend-aasland - I think everything is addressed now. Would you please have another look?

Copy link
Contributor

@erlend-aasland erlend-aasland left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@erlend-aasland
Copy link
Contributor

I'm inclined to run the buildbots on this; Thomas is cooking up the release right now, so we really don't want one of the bots to turn red right now.

@erlend-aasland erlend-aasland added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Feb 14, 2024
@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @erlend-aasland for commit 6ffad44 🤖

If you want to schedule another build, you need to add the 🔨 test-with-buildbots label again.

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Feb 14, 2024
@mpage
Copy link
Contributor Author

mpage commented Feb 14, 2024

@erlend-aasland - Just noticed one last place where we want to use the macro wrapper. Would you please kick off another buildbot run. Sorry for the churn.

@AlexWaygood
Copy link
Member

@erlend-aasland - Just noticed one last place where we want to use the macro wrapper. Would you please kick off another buildbot run. Sorry for the churn.

You should be able to trigger the buildbot run yourself, now you're a triager! :)

@mpage mpage added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Feb 14, 2024
@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @mpage for commit fa7e8a8 🤖

If you want to schedule another build, you need to add the 🔨 test-with-buildbots label again.

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Feb 14, 2024
@mpage
Copy link
Contributor Author

mpage commented Feb 14, 2024

You should be able to trigger the buildbot run yourself, now you're a triager! :)

Oho - I'm drunk with power now! :P

Copy link
Member

@corona10 corona10 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@erlend-aasland erlend-aasland merged commit dc978f6 into python:main Feb 15, 2024
118 of 119 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants