gh-144191: Dataclasses single field ordering#144222
gh-144191: Dataclasses single field ordering#144222whyvineet wants to merge 6 commits intopython:mainfrom
Conversation
|
Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool. If this change has little impact on Python users, wait for a maintainer to apply the |
johnslavik
left a comment
There was a problem hiding this comment.
Thanks! You can wait for the green light from Eric before going forward or add tests for this even now. I'd probably change test_1_field_compare. And of course a news entry, too.
|
Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool. If this change has little impact on Python users, wait for a maintainer to apply the |
|
Hey @picnixz and @johnslavik, just a quick update: I ran a small local benchmark on a CPython 3.15 dev build to ensure this change doesn't cause any noticeable slowdown. In my tests, the I’m happy to follow your advice on this. If you’d rather test it yourselves or revert to the |
|
Just for reference, here are the numbers I observed locally (CPython 3.15 dev build):
These were consistent across repeated runs. |
|
Please share the benchmarking script and the way you ran it. Class creation matters when considering import time as well. |
|
The script was executed using the freshly built interpreter. Each implementation ( Scriptimport timeit
import sys
from dataclasses import dataclass
class MockField:
def __init__(self, name):
self.name = name
def _tuple_str(obj_name, flds):
if not flds:
return '()'
return f'({",".join(f"{obj_name}.{f.name}" for f in flds)},)'
def match_impl(flds):
match flds:
case [single_fld]:
self_expr = f"self.{single_fld.name}"
other_expr = f"other.{single_fld.name}"
case _:
self_expr = _tuple_str("self", flds)
other_expr = _tuple_str("other", flds)
return self_expr, other_expr
def ifelse_impl(flds):
if len(flds) == 1:
self_expr = f"self.{flds[0].name}"
other_expr = f"other.{flds[0].name}"
else:
self_expr = _tuple_str("self", flds)
other_expr = _tuple_str("other", flds)
return self_expr, other_expr
def run_benchmark():
single_field = [MockField("value")]
multi_field = [MockField("x"), MockField("y"), MockField("z")]
iterations = 1_000_000
# Just to make sure that I am using the local build
print("Python:", sys.version)
print("Executable:", sys.executable)
print()
print("Single-field case")
print("match/case:", timeit.timeit(lambda: match_impl(single_field), number=iterations))
print("if/else:", timeit.timeit(lambda: ifelse_impl(single_field), number=iterations))
print("Multi-field case")
print("match/case:", timeit.timeit(lambda: match_impl(multi_field), number=iterations))
print("if/else:", timeit.timeit(lambda: ifelse_impl(multi_field), number=iterations))
print("Real dataclass comparison (sanity check)")
@dataclass(order=True)
class A:
x: int
@dataclass(order=True)
class B:
x: int
y: int
z: int
a1, a2 = A(1), A(2)
b1, b2 = B(1, 2, 3), B(2, 3, 4)
print("single-field:", timeit.timeit(lambda: a1 < a2, number=iterations))
print("multi-field: ", timeit.timeit(lambda: b1 < b2, number=iterations))
if __name__ == "__main__":
run_benchmark() |
|
Can you use pyperf instead of custom benchmarks? I am also interested in the stdev. |
|
@picnixz, Please excuse any rough edges here... I’m still getting familiar with Scriptimport pyperf
class MockField:
def __init__(self, name):
self.name = name
def _tuple_str(obj_name, flds):
if not flds:
return '()'
return f'({",".join(f"{obj_name}.{f.name}" for f in flds)},)'
def match_impl(flds):
match flds:
case [single_fld]:
self_expr = f"self.{single_fld.name}"
other_expr = f"other.{single_fld.name}"
case _:
self_expr = _tuple_str("self", flds)
other_expr = _tuple_str("other", flds)
return self_expr, other_expr
def ifelse_impl(flds):
if len(flds) == 1:
self_expr = f"self.{flds[0].name}"
other_expr = f"other.{flds[0].name}"
else:
self_expr = _tuple_str("self", flds)
other_expr = _tuple_str("other", flds)
return self_expr, other_expr
runner = pyperf.Runner()
single_field = [MockField("value")]
runner.bench_func(
"dataclasses single-field match/case",
match_impl,
single_field
)
runner.bench_func(
"dataclasses single-field if/else",
ifelse_impl,
single_field
) |
|
I would like the results with the dataclass usage. And with more than just 3 fields. Is the interpreter built with PGO and LTO? Or is it a debug build? |
|
@picnixz, Here are the results with dataclass usage (I'm using a debug build) match-casesif-elseScriptimport pyperf
from dataclasses import dataclass
def make_dataclass(n):
namespace = {f"x{i}": int for i in range(n)}
return dataclass(order=True)(
type(f"C{n}", (), {"__annotations__": namespace})
)
runner = pyperf.Runner()
runner.bench_func("dataclass creation (1 field)", make_dataclass, 1)
runner.bench_func("dataclass creation (5 fields)", make_dataclass, 5)
runner.bench_func("dataclass creation (10 fields)", make_dataclass, 10) |
|
Results on DEBUG builds are not relevant. Please use a PGO/LTO build. |
|
In addition can you use pyperf compare as well? And if possible remove the time needed for creating the type. We are only interested in the time needed by the decorator. |
|
@picnixz, I ran |
|
Arf, actually I don't know if we can make that change. It's a breaking change because of dataclasses wrapping a single float for instance with So I think we need more discussion on the issue. Sorry. But for now, I'd suggest that we close this PR. It's not that it's bad, it's just that this can (once again) give surprises (in particular, I want to hear from more core devs). If we make the change, it can only go into 3.15. People may have already noticed the problem that was introduced so fixing it again would make it worse for them (maybe, idk). At least, I would prefer to do so. |
Simplify single-field dataclass ordering comparisons
#144191