| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
|
Most changes to Python require a NEWS entry. Please add it using the blurb_it web app or the blurb command-line tool. |
Sorry, something went wrong.
Sorry, something went wrong.
|
The issue-number CI check is failing because gh-102573 does not yet exist as a GitHub issue :) You should create an issue to describe the change you're proposing here, and then link to it in the title of this PR. |
Sorry, something went wrong.
|
Opened a linked issue and added some benchmark results. |
Sorry, something went wrong.
Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
|
Looks like the CLA check has started failing with the latest commit -- if you're using two email address, you may have to sign it with both email addresses, unfortunately :( |
Sorry, something went wrong.
|
Seems that the CLA check has passed now. |
Sorry, something went wrong.
There was a problem hiding this comment.
Looks great to me. I verified the speedup by running this benchmark locally (which is a little different to the one you posted in your issue and the one you posted in this PR).
Benchmark:import dataclasses
import string
import time
Foo = dataclasses.make_dataclass(
"Foo",
[(letter, int) for letter in string.ascii_lowercase],
frozen=True
)
class Bar(Foo): ...
instance = Bar(*range(26))
t0 = time.perf_counter()
for _ in range(10_000_000):
instance.foo = 1
del instance.foo
print(f"{time.perf_counter() - t0:.2f}")The result of this benchmark is 15.15 seconds on main on my machine (--pgo non-debug build), but 6.44 seconds with this patch applied.
I'll wait for a thumbs-up from @ericvsmith, @carljm, or another core dev before merging, but this has my approval -- thanks!
Sorry, something went wrong.
| else: | ||
| # Special case for the zero-length tuple. | ||
| # Special case for the zero-length set. | ||
| # Use the empty tuple singleton to avoid unnecessary `set` construction |
There was a problem hiding this comment.
Not that it matters much, but the zero length case could just avoid the or name in ... test entirely. Maybe fields_str should become fields_test, and then set it to or name in {<<generated set literal>>} or set it to an empty string if there are no fields. Then change the generated code to f'if type(self) is cls {fields_test}:' Although that doesn't read very well. Maybe tweak fields_test to be something else.
This could be part of a different PR, or include it here. But in any event I'm not positive that the zero length case actually has a test. We should make sure it does for this PR.
Sorry, something went wrong.
There was a problem hiding this comment.
But in any event I'm not positive that the zero length case actually has a test. We should make sure it does for this PR.
Added a small test for empty frozen dataclass.
Sorry, something went wrong.
There was a problem hiding this comment.
Code changes LGTM. A couple comments on the new test.
Sorry, something went wrong.
|
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request. And if you don't make the requested changes, you will be put in the comfy chair! |
Sorry, something went wrong.
|
I have made the requested changes; please review again |
Sorry, something went wrong.
|
Thanks for making the requested changes! @carljm, @AlexWaygood: please review the changes made to this pull request. |
Sorry, something went wrong.
There was a problem hiding this comment.
Looks good to me; thanks for the perf improvement!
Sorry, something went wrong.
There was a problem hiding this comment.
Looks great!
Sorry, something went wrong.
…non-dataclass subclasses of frozen dataclasses (pythongh-102573)
| Back | FazBrowse Home | New Git URL |
Creating dataclasses with argument frozen=True will automatically generate methods __setattr__ and __delattr__ in _frozen_get_del_attr.
This PR changes tuple-based lookup to set-based lookup. Reduce the time complexity from $O(n)$ to $O(1)$.
A tiny benchmark script:
Result:
set-based lookup:
tuple-based lookup (original):
The set-based is constantly faster than the old approach. And the theoretical time complexity is also smaller ($O(1)$ vs. $O(n)$).
Resolves #102578