Skip to content

Conversation

eendebakpt
Copy link
Contributor

@eendebakpt eendebakpt commented Aug 28, 2025

Benchmark results with PGO on Linux:

asdict: Mean +- std dev: [main2] 2.13 us +- 0.10 us -> [pr2] 1.61 us +- 0.11 us: 1.32x faster
astuple: Mean +- std dev: [main2] 2.61 us +- 0.14 us -> [pr2] 2.15 us +- 0.12 us: 1.21x faster
f.__getstate__(): Mean +- std dev: [main2] 840 ns +- 41 ns -> [pr2] 339 ns +- 20 ns: 2.48x faster

Benchmark hidden because not significant (1): instance creation

Geometric mean: 1.41x faster

(script in issue)

Copy link
Member

@picnixz picnixz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few nitpicks. Memory-wise, we're already holding all the fields in _FIELDS, so holding their names shouldn't be an issue. Don't forget the NEWS + What's New entry if the change is accepted.

# order, so the order of the tuple is as the fields were defined.
return tuple(f for f in fields.values() if f._field_type is _FIELD)

def _field_names(class_or_instance):
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Inlining this method gives a bit of performance gain:

%timeit _field_names(s)
%timeit s.__dataclass_field_names__

Results in

41.8 ns ± 1.59 ns per loop (mean ± std. dev. of 7 runs, 10,000,000 loops each)
17.8 ns ± 0.188 ns per loop (mean ± std. dev. of 7 runs, 100,000,000 loops each)

@eendebakpt eendebakpt requested a review from picnixz September 10, 2025 18:47
Copy link
Member

@picnixz picnixz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

More generally, I wonder whether many small functions would actually benefit from a C implementation as they seem to target micro-optimizations. I don't know which ones we could optimize and I also don't know if it's worth it but had we ever considered doing this?

Now, I think it's a good trade-off to increase memory per dataclass type but reduce quite drastically the time it takes to do asdict transformations.

Accepts a dataclass or an instance of one. Excludes pseudo-fields.
"""

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My suggestion here is to remove the blank line after """. And I however think that it would be better to actually inline the function but considering we also use this approach for _PARAMS, it would be more consistent to keep it.

@eendebakpt
Copy link
Contributor Author

I addressed the review comments. In the last commit I inlined the _field_names method. That can be reverted though if needed.

# also marks this class as being a dataclass.
setattr(cls, _FIELDS, fields)
# Store field names. Excludes pseudo-fields.
setattr(cls, _FIELD_NAMES, tuple(f.name for f in fields.values()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you want, you can also do cls.__dataclass_field_names__ = ... directly (I would also advise you to consider this as part of the "inline" commit, so you can also force-push this one).

An other alternative is to only inline getattr(x, _FIELDS_NAMES) instead of having a standalone function for that. It could also improve readability a bit and justify the needs of the global _FIELD_NAMES.

IOW, choose among the following:

  • Hardcode __dataclass_field_names__ everywhere, without having using getattr/setattr and _FIELD_NAMES.
  • Use _FIELD_NAMES + getattr/setattr(..., _FIELD_NAMES) directly.
  • Use _FIELD_NAMES + a module-wide function defined to be the partialization _get_names(x) ~ getattr(x, _FIELD_NAMES).

My preference is (1) or (2) but (3) is an overkill IMO.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, going for option (1)

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