Skip to content

Performance optimization for dataclass order methods #137497

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

gesslerpd
Copy link
Contributor

Similar to #104904

This can speed up ordering/comparison operations by ~2x by making the comparison similar to existing __eq__ performance.

Benchmark timings for classes with 5 integer fields:

equality (ms) order (ms)
standard class w/ optimization 46.19 47.27
existing dataclasses 45.96 114.22

@gesslerpd gesslerpd requested a review from ericvsmith as a code owner August 7, 2025 00:31
@gesslerpd gesslerpd changed the title gh-*: Performance optimization for dataclass order methods gh-137497: Performance optimization for dataclass order methods Aug 7, 2025
@AA-Turner AA-Turner changed the title gh-137497: Performance optimization for dataclass order methods Performance optimization for dataclass order methods Aug 7, 2025
@AA-Turner
Copy link
Member

AA-Turner commented Aug 7, 2025

Don't use a PR number as the issue prefix, things get confused. Please can you create an issue for this PR, and add a NEWS entry?

Please can you use a pyperformance benchmark, and update the PR with the benchmark code and results? Microbenchmark results should show a reasonably large performance improvement, which this PR hopefully will based on the preliminary numbers you've shown.

A

@ericvsmith
Copy link
Member

We need to be very careful that we're not changing semantics with this. We've had that problem before with comparisons (although of course now I can't find the issue). I don't think we added tests for this, because we didn't find the problem until after we'd released the code.

@gesslerpd
Copy link
Contributor Author

@ericvsmith so we need set of tests that pass prior to this change?

Potentially relevant "semantics" you mention may be shown in the tests for this change in #582?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants