Skip to content
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.

Sign up for GitHub

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

Jump to bottom

streamable: Cache convert functions from dataclass_from_dict #10561

Merged
merged 4 commits into from
May 24, 2022
Merged

streamable: Cache convert functions from dataclass_from_dict #10561

merged 4 commits into from
May 24, 2022

Conversation

xdustinface
Copy link
Contributor

Applies the pattern from #10419 to dataclass_from_dict i.e. it caches the conversion functions on startup which leads to nice improvements of the from_json benchmark (Calling Class.from_json_dict). Not so much for full_block, i guess because the subgroup checks of the BLS deserialisation still dominate.

compare: benchmark, old: 4bd5c53f4-dirty, new: 51f97c79c-dirty
mode         | µs/iteration old | µs/iteration new | diff %      
from_json    | 1091.09          | 568.4            | -47.9       

compare: full_block, old: 4bd5c53f4-dirty, new: 51f97c79c-dirty
mode         | µs/iteration old | µs/iteration new | diff %      
from_json    | 3090.49          | 2739.37          | -11.36

It also adds some more tests where dataclass_from_dict gets used with non-streamable dataclasses because there is a special handling of them now in there where the cache gets created on the fly (we can't do it on startup for non-streamable dataclasses because of the missing decorator call).

https://github.com/Chia-Network/chia-blockchain/blob/5b1b4ddd9d3b72f66ef57e42528c1f9674b869d3/chia/util/streamable.py#L124-L135

Based on #10539

@lgtm-com
Copy link

lgtm-com bot commented Mar 5, 2022

This pull request fixes 1 alert when merging 51f97c79c9a6276b8e91287dd791118b046e3674 into 4bd5c53 - view on LGTM.com

fixed alerts:

  • 1 for Unused import

@xdustinface xdustinface force-pushed the pr-streamable-cache-for-dataclass-from-dict branch from 51f97c7 to d304440 Compare March 10, 2022 17:54
@xdustinface xdustinface mentioned this pull request Mar 10, 2022
Merged
@lgtm-com
Copy link

lgtm-com bot commented Mar 10, 2022

This pull request fixes 1 alert when merging d304440b3c352134df7d8f903583e1917e176ad9 into 0e29dbc - view on LGTM.com

fixed alerts:

  • 1 for Unused import

@xdustinface xdustinface force-pushed the pr-streamable-cache-for-dataclass-from-dict branch from d304440 to b55b64a Compare March 10, 2022 18:16
@lgtm-com
Copy link

lgtm-com bot commented Mar 10, 2022

This pull request fixes 1 alert when merging b55b64ae97a243a23c527397a15acdf7661cc33e into 0e29dbc - view on LGTM.com

fixed alerts:

  • 1 for Unused import

@github-actions
Copy link
Contributor

This PR has been flagged as stale due to no activity for over 60 days. It will not be automatically closed, but it has been given a stale-pr label and should be manually reviewed by the relevant parties.

@github-actions github-actions bot added the stale-pr Flagged as stale and in need of manual review label Apr 25, 2022
@xdustinface xdustinface force-pushed the pr-streamable-cache-for-dataclass-from-dict branch from b55b64a to 6be309b Compare April 27, 2022 10:26
@xdustinface xdustinface marked this pull request as ready for review April 27, 2022 10:26
@xdustinface xdustinface removed the stale-pr Flagged as stale and in need of manual review label Apr 29, 2022
@github-actions
Copy link
Contributor

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions github-actions bot added the merge_conflict Branch has conflicts that prevent merge to main label May 16, 2022
@xdustinface xdustinface force-pushed the pr-streamable-cache-for-dataclass-from-dict branch from 6be309b to 01d8712 Compare May 17, 2022 14:09
@github-actions github-actions bot removed the merge_conflict Branch has conflicts that prevent merge to main label May 17, 2022
@github-actions
Copy link
Contributor

Conflicts have been resolved. A maintainer will review the pull request shortly.

@mariano54
Copy link
Contributor

@arvidn is making changes to the same code, so it's probably good to coordinate

@xdustinface
Copy link
Contributor Author

@mariano54 Do you mean #11537? If so, thats to_json but this PR here is about from_json. Or do you have other stuff in progress about dataclass_from_dict @arvidn ?

@mariano54
Copy link
Contributor

@mariano54 Do you mean #11537? If so, thats to_json but this PR here is about from_json. Or do you have other stuff in progress about dataclass_from_dict @arvidn ?

Yes that is the one I was referring to.

altendky
altendky reviewed May 19, 2022
View reviewed changes
chia/util/streamable.py Outdated Show resolved Hide resolved
chia/util/streamable.py Outdated Show resolved Hide resolved
chia/util/streamable.py Show resolved Hide resolved
xdustinface and others added 2 commits May 20, 2022 00:41
@xdustinface @altendky
Any -> object
2c723dc 
Co-authored-by: Kyle Altendorf <sda@fstab.net>
@xdustinface
Move comment into dataclass_from_dict
439ed9c
@xdustinface xdustinface requested a review from altendky May 23, 2022 10:02
@xdustinface xdustinface added the ready_to_merge Submitter and reviewers think this is ready label May 23, 2022
@wjblanke wjblanke merged commit dda517e into Chia-Network:main May 24, 2022
@xdustinface xdustinface mentioned this pull request Jun 1, 2022
Merged
xdustinface added a commit to xdustinface/chia-blockchain that referenced this pull request Jun 3, 2022
@xdustinface
streamable: Use constructor in dataclass_from_dict
1edbfa0 
This fixes default value assignments after Chia-Network#10561 but also leads to less perfomance due to `__post_init__` being called which at least gets mitigated by Chia-Network#11730.
wallentx pushed a commit that referenced this pull request Jun 3, 2022
@xdustinface
streamable: Fix default value assignments for dataclass_from_dict (#…
5c5ab11 
…11732)

* streamable: Use constructor in `dataclass_from_dict`

This fixes default value assignments after #10561 but also leads to less perfomance due to `__post_init__` being called which at least gets mitigated by #11730.

* tests: Test default values with `from_json_dict`

* Convert to `str`, then compare.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@altendky altendky altendky approved these changes

@mariano54 mariano54 Awaiting requested review from mariano54

Assignees
No one assigned
Labels
ready_to_merge Submitter and reviewers think this is ready
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@xdustinface @mariano54 @altendky @wjblanke