-
-
Notifications
You must be signed in to change notification settings - Fork 29.8k
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-121795: Improve performance of set membership testing from set arguments #121796
Conversation
HarryLHW
commented
Jul 15, 2024
•
edited by bedevere-app
bot
Loading
edited by bedevere-app
bot
- Issue: Improve performance of set membership testing from set arguments #121795
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.
Can we have some benchmarks?
Yes :) Benchmark (on an M2 Macbook Air): Script:
main branch result:
my branch result:
There will be no performance regression on the normal case. |
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.
Maybe like this? (for the rest, I'll leave it to Raymond)
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.
This is a neat performance improvement, that contrary to most other such attempts, does not add to the complexity of the code; nice. AFAICS, this is good to go. I'll leave the landing of the PR to the code owner.
Thank you so much for your review. I have made changes to resolve the issues. |
We used to do something like this, but it caused bugs and had to be removed. See https://bugs.python.org/issue8757 and checkin 4d45c10 . IIRC there was also a reason that the set had to be made temporarily immutable during the lookup. I don't remember all the details now. Perhaps @serhiy-storchaka does. The original |
It was before I started contributing to CPython, so I have no memories about this case. The approach of this PR is better than the code used before bpo-8757. It leaves the original set key unmodified, so other threads are not affected if they only read it. The original reproducer should pass this test. But what if the set key is modified in other thread? I think this is safe if set comparison is thread-safe (and AFAIK it is). There is a new race condition: the set key can be changed during calculating its hash (currently creating a frozenset from a set is atomic, or it can be made atomic). But I do not think this is bad. We should not guarantee the correct result in such case. I think this change is safe. LGTM. |