GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | ||||||||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | ||||||||||||||||||||
GeezerVison is showing github.com — gh-102578: Optimise setting and deleting mutable attributes on non-dataclass subclasses of frozen dataclasses by XuehaiPan · Pull Request #102573 · python/cpython Conversation
|
|
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.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
set-based name lookup rather than tuples for frozen dataclassesset-based name lookup rather than tuples for frozen dataclasses
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
|
The 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.
There was an error while loading. Please reload this page.
set-based name lookup rather than tuples for frozen dataclassesset-based name lookup rather than tuples for frozen dataclasses
|
Opened a linked issue and added some benchmark results. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
Co-authored-by: Alex Waygood <[email protected]>
|
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.
There was an error while loading. Please reload this page.
|
Seems that the CLA check has passed now. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
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).
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.
There was an error while loading. Please reload this page.
set-based name lookup rather than tuples for frozen dataclasses| 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.
The reason will be displayed to describe this comment to others. Learn more.
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 an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
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 an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Code changes LGTM. A couple comments on the new test.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
|
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 And if you don't make the requested changes, you will be put in the comfy chair! |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
I have made the requested changes; please review again |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Thanks for making the requested changes! @carljm, @AlexWaygood: please review the changes made to this pull request. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me; thanks for the perf improvement!
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Looks great!
Sorry, something went wrong.
There was an error while loading. Please reload this page.
…non-dataclass subclasses of frozen dataclasses (pythongh-102573)
There was an error while loading. Please reload this page.
Successfully merging this pull request may close these issues.
[Enhancement] Speed up setting and deleting mutable attributes on non-dataclass subclasses of frozen dataclasses
There was an error while loading. Please reload this page.