-
-
Notifications
You must be signed in to change notification settings - Fork 29.1k
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-116621: Specialize list.extend for dict keys/values #116816
Conversation
Lib/test/test_ordered_dict.py
Outdated
@@ -568,14 +568,22 @@ def __hash__(self): | |||
od[key] = i | |||
|
|||
# These should not crash. | |||
with self.assertRaises(KeyError): | |||
try: |
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.
@colesbury
I noticed that this change has some behavior changes.(in nogil 3.12, it has same behavior changes)
I'm not sure it's worth applying it.
See: colesbury/nogil-3.12@da071fa also
Let me check a microbenchmark instead.
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.
From my local benchmark, for the default build, it makes 2% slower.
(Or we can handle it as a noise)
(.venv) ➜ cpython git:(gh-116621-more) ✗ python -m pyperf compare_to base.json opt.json
Mean +- std dev: [base] 36.3 us +- 0.7 us -> [opt] 36.8 us +- 0.6 us: 1.02x slower
import pyperf
runner = pyperf.Runner()
runner.timeit(name="bench list.extend keys",
stmt="""
keys = list(d.keys())
""",
setup = """
d = dict()
for k in range(10000):
d[k] = k
"""
)
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.
There are a few other cases in nogil-3.12 that we probably want to handle atomically as well, because I think we use the same pattern where the calling code assumes list(some_type) is atomic for certain built-in types:
When considering your suggestion, IMO, we should apply this technique to the free-threaded build only, WDYT?
And please let me know if this is not a fair benchmark.
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.
Regarding the OrderedDict
changes: let's check for the exact type PyDictKeys_Type
and PyDictValues_Type
. I'm not sure using PyDict_Next
is always appropriate for subclasses.
On the benchmark, I think that's just noise. I was not able to reproduce that locally. Avoiding the iterator creation in the specialized case is going to be faster when the dict has few items.
Lib/test/test_ordered_dict.py
Outdated
@@ -568,14 +568,22 @@ def __hash__(self): | |||
od[key] = i | |||
|
|||
# These should not crash. | |||
with self.assertRaises(KeyError): | |||
try: |
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.
Regarding the OrderedDict
changes: let's check for the exact type PyDictKeys_Type
and PyDictValues_Type
. I'm not sure using PyDict_Next
is always appropriate for subclasses.
On the benchmark, I think that's just noise. I was not able to reproduce that locally. Avoiding the iterator creation in the specialized case is going to be faster when the dict has few items.
Thanks for the review, I will take a look at tomorrow :) |
Unrelated to this PR. |
list(set)
should be atomic in the free-threaded build #116621