GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

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

@XuehaiPan

@XuehaiPan XuehaiPan commented Mar 10, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

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)$.

In [1]: # tuple-based

In [2]: %timeit 'a' in ('a', 'b', 'c', 'd', 'e', 'f', 'g')
9.91 ns ± 0.0982 ns per loop (mean ± std. dev. of 7 runs, 100,000,000 loops each)

In [3]: %timeit 'd' in ('a', 'b', 'c', 'd', 'e', 'f', 'g')
33.2 ns ± 0.701 ns per loop (mean ± std. dev. of 7 runs, 10,000,000 loops each)

In [4]: %timeit 'g' in ('a', 'b', 'c', 'd', 'e', 'f', 'g')
56.4 ns ± 0.818 ns per loop (mean ± std. dev. of 7 runs, 10,000,000 loops each)

In [5]: # set-based

In [6]: %timeit 'a' in {'a', 'b', 'c', 'd', 'e', 'f', 'g'}
11.3 ns ± 0.0723 ns per loop (mean ± std. dev. of 7 runs, 100,000,000 loops each)

In [7]: %timeit 'd' in {'a', 'b', 'c', 'd', 'e', 'f', 'g'}
11 ns ± 0.106 ns per loop (mean ± std. dev. of 7 runs, 100,000,000 loops each)

In [8]: %timeit 'g' in {'a', 'b', 'c', 'd', 'e', 'f', 'g'}
11.1 ns ± 0.126 ns per loop (mean ± std. dev. of 7 runs, 100,000,000 loops each)

A tiny benchmark script:

from contextlib import suppress
from dataclasses import FrozenInstanceError, dataclass

@dataclass(frozen=True)
class Foo2:
    a: int
    b: int

foo2 = Foo2(1, 2)

def bench2(inst):
    with suppress(FrozenInstanceError):
        inst.a = 0
    with suppress(FrozenInstanceError):
        inst.b = 0

@dataclass(frozen=True)
class Foo7:
    a: int
    b: int
    c: int
    d: int
    e: int
    f: int
    g: int

foo7 = Foo7(1, 2, 3, 4, 5, 6, 7)

def bench7(inst):
    with suppress(FrozenInstanceError):
        inst.a = 0
    with suppress(FrozenInstanceError):
        inst.b = 0
    with suppress(FrozenInstanceError):
        inst.c = 0
    with suppress(FrozenInstanceError):
        inst.d = 0
    with suppress(FrozenInstanceError):
        inst.e = 0
    with suppress(FrozenInstanceError):
        inst.f = 0
    with suppress(FrozenInstanceError):
        inst.g = 0

class Bar(Foo7):
    def __init__(self, a, b, c, d, e, f, g):
        super().__init__(a, b, c, d, e, f, g)
        self.baz = 0

def bench(inst):
    inst.baz = 1

Result:

set-based lookup:

In [2]: %timeit bench2(foo2)
1.08 µs ± 28.1 ns per loop (mean ± std. dev. of 7 runs, 1,000,000 loops each)

In [3]: %timeit bench7(foo7)
3.81 µs ± 20.3 ns per loop (mean ± std. dev. of 7 runs, 100,000 loops each)

In [4]: %timeit bench(bar)
249 ns ± 6.31 ns per loop (mean ± std. dev. of 7 runs, 1,000,000 loops each)

tuple-based lookup (original):

In [2]: %timeit bench2(foo2)
1.15 µs ± 10.9 ns per loop (mean ± std. dev. of 7 runs, 1,000,000 loops each)

In [3]: %timeit bench7(foo7)
3.97 µs ± 15.7 ns per loop (mean ± std. dev. of 7 runs, 100,000 loops each)

In [4]: %timeit bench(bar)
269 ns ± 4.09 ns per loop (mean ± std. dev. of 7 runs, 1,000,000 loops each)

The set-based is constantly faster than the old approach. And the theoretical time complexity is also smaller ($O(1)$ vs. $O(n)$).

  • Issue: [Enhancement] Speed up setting and deleting mutable attributes on non-dataclass subclasses of frozen dataclasses #102578

Resolves #102578

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@XuehaiPan
Use set-based name lookup rather than tuples for frozen dataclasses
776843d
@XuehaiPan
XuehaiPan requested a review from ericvsmith as a code owner March 10, 2023 09:57
@bedevere-bot

bedevere-bot commented Mar 10, 2023

Copy link
Copy Markdown

Most changes to Python require a NEWS entry.

Please add it using the blurb_it web app or the blurb command-line tool.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@bedevere-bot bedevere-bot added the awaiting review label Mar 10, 2023
@ghost

ghost commented Mar 10, 2023 •
edited by ghost
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown

All commit authors signed the Contributor License Agreement.
CLA signed

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

XuehaiPan added 2 commits March 10, 2023 10:06
@XuehaiPan
Merge branch 'main' into dataclasses-lookup
0a400c1
@XuehaiPan
Add news entry
54dd9c3
@XuehaiPan XuehaiPan changed the title Use set-based name lookup rather than tuples for frozen dataclasses gh-102573: Use set-based name lookup rather than tuples for frozen dataclasses Mar 10, 2023
@XuehaiPan
Update new entry
845c8ff
TeamSpen210
TeamSpen210 reviewed Mar 10, 2023
View reviewed changes
Comment thread Lib/dataclasses.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

@XuehaiPan
Use empty tuple singleton in simple case
0f0d97c
AlexWaygood
AlexWaygood reviewed Mar 10, 2023
View reviewed changes
Comment thread Misc/NEWS.d/next/Library/2023-03-10-10-10-00.gh-issue-102573.GQ7sV6.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

@AlexWaygood

AlexWaygood commented Mar 10, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@XuehaiPan XuehaiPan mentioned this pull request Mar 10, 2023
[Enhancement] Speed up setting and deleting mutable attributes on non-dataclass subclasses of frozen dataclasses #102578
Closed
@XuehaiPan XuehaiPan changed the title gh-102573: Use set-based name lookup rather than tuples for frozen dataclasses gh-102578: Use set-based name lookup rather than tuples for frozen dataclasses Mar 10, 2023
@XuehaiPan
Update new entry
0524ee4
@XuehaiPan

XuehaiPan commented Mar 10, 2023

Copy link
Copy Markdown
Contributor Author

Opened a linked issue and added some benchmark results.

AlexWaygood reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

AlexWaygood
AlexWaygood reviewed Mar 10, 2023
View reviewed changes
Comment thread Misc/NEWS.d/next/Library/2023-03-10-13-21-16.gh-issue-102578.-gujoI.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

@XuehaiPan
Add more details about the change in news entry
4055fa3
AlexWaygood
AlexWaygood reviewed Mar 10, 2023
View reviewed changes
Comment thread Misc/NEWS.d/next/Library/2023-03-10-13-21-16.gh-issue-102578.-gujoI.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

XuehaiPan and others added 2 commits March 10, 2023 22:08
@XuehaiPan @AlexWaygood
Update news entry
0bbf3a8
Co-authored-by: Alex Waygood <[email protected]>
@XuehaiPan
Merge branch 'main' into dataclasses-lookup
05eae2c
@AlexWaygood AlexWaygood added performance Performance or resource usage stdlib Standard Library Python modules in the Lib/ directory labels Mar 10, 2023
@AlexWaygood

AlexWaygood commented Mar 10, 2023

Copy link
Copy Markdown
Member

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 :(

XuehaiPan reacted with heart emoji
All reactions
  • ❤️ 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@XuehaiPan

XuehaiPan commented Mar 10, 2023

Copy link
Copy Markdown
Contributor Author

Seems that the CLA check has passed now.

AlexWaygood reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

AlexWaygood
AlexWaygood approved these changes Mar 10, 2023
View reviewed changes

@AlexWaygood AlexWaygood left a comment •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

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.

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.

Uh oh!

There was an error while loading. Please reload this page.

XuehaiPan reacted with heart emoji
All reactions
  • ❤️ 1 reaction
@bedevere-bot bedevere-bot added awaiting merge and removed awaiting review labels Mar 10, 2023
@AlexWaygood
AlexWaygood requested a review from carljm March 10, 2023 14:27
@AlexWaygood AlexWaygood changed the title gh-102578: Use set-based name lookup rather than tuples for frozen dataclasses gh-102578: Optimise setting and deleting mutable attributes on non-dataclass subclasses of frozen dataclasses Mar 10, 2023
ericvsmith
ericvsmith reviewed Mar 10, 2023
View reviewed changes
Comment thread Lib/dataclasses.py Outdated
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

@ericvsmith ericvsmith Mar 10, 2023

Copy link
Copy Markdown
Member

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.

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.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@XuehaiPan XuehaiPan Mar 10, 2023

Copy link
Copy Markdown
Contributor Author

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.

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.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@XuehaiPan
Eliminate unnecessary check for empty fields
b90d251
AlexWaygood
AlexWaygood reviewed Mar 10, 2023
View reviewed changes
Comment thread Lib/dataclasses.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

XuehaiPan added 3 commits March 11, 2023 00:11
@XuehaiPan
Remove mistake from auto import
d3c8403
@XuehaiPan
Add test for empty frozen dataclass
84b5bdd
@XuehaiPan
Add test for empty frozen dataclass
2d38e9c
carljm
carljm requested changes Mar 10, 2023
View reviewed changes

@carljm carljm left a comment

Copy link
Copy Markdown
Member

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.

Code changes LGTM. A couple comments on the new test.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread Lib/test/test_dataclasses.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Lib/test/test_dataclasses.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

@bedevere-bot bedevere-bot added awaiting changes and removed awaiting merge labels Mar 10, 2023
@bedevere-bot

bedevere-bot commented Mar 10, 2023

Copy link
Copy Markdown

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!

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@XuehaiPan
Add test for empty frozen dataclass
ef8e83f
@XuehaiPan

XuehaiPan commented Mar 10, 2023

Copy link
Copy Markdown
Contributor Author

I have made the requested changes; please review again

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@bedevere-bot bedevere-bot added awaiting change review and removed awaiting changes labels Mar 10, 2023
@bedevere-bot

bedevere-bot commented Mar 10, 2023

Copy link
Copy Markdown

Thanks for making the requested changes!

@carljm, @AlexWaygood: please review the changes made to this pull request.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@bedevere-bot
bedevere-bot requested review from AlexWaygood and carljm March 10, 2023 16:51
XuehaiPan
XuehaiPan commented Mar 10, 2023
View reviewed changes
Comment thread Lib/test/test_dataclasses.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

carljm
carljm approved these changes Mar 10, 2023
View reviewed changes

@carljm carljm left a comment

Copy link
Copy Markdown
Member

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.

Looks good to me; thanks for the perf improvement!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@bedevere-bot bedevere-bot added awaiting merge and removed awaiting change review labels Mar 10, 2023
@XuehaiPan
Add extra check for the exception type
5da787e
AlexWaygood
AlexWaygood approved these changes Mar 10, 2023
View reviewed changes

@AlexWaygood AlexWaygood left a comment

Copy link
Copy Markdown
Member

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.

Looks great!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@ericvsmith
ericvsmith merged commit ee6f841 into python:main Mar 11, 2023
@bedevere-bot bedevere-bot removed the awaiting merge label Mar 11, 2023
@XuehaiPan
XuehaiPan deleted the dataclasses-lookup branch March 11, 2023 04:44
iritkatriel pushed a commit to iritkatriel/cpython that referenced this pull request Mar 12, 2023
@XuehaiPan @iritkatriel
pythongh-102578: Optimise setting and deleting mutable attributes on …
b85d003
…non-dataclass subclasses of frozen dataclasses (pythongh-102573)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ericvsmith ericvsmith ericvsmith left review comments
@carljm carljm carljm approved these changes
@AlexWaygood AlexWaygood AlexWaygood approved these changes
+1 more reviewer
@TeamSpen210 TeamSpen210 TeamSpen210 left review comments
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

performance Performance or resource usage stdlib Standard Library Python modules in the Lib/ directory

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[Enhancement] Speed up setting and deleting mutable attributes on non-dataclass subclasses of frozen dataclasses

Uh oh!

There was an error while loading. Please reload this page.

6 participants

@XuehaiPan @bedevere-bot @AlexWaygood @carljm @ericvsmith @TeamSpen210
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.