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: Introduce Streamable.__post_init__ processing cache #11730

Merged
merged 1 commit into from
Jun 28, 2022
Merged

streamable: Introduce Streamable.__post_init__ processing cache #11730

merged 1 commit into from
Jun 28, 2022

Conversation

xdustinface
Copy link
Contributor

@xdustinface xdustinface commented Jun 1, 2022 •
edited
Loading

Speeds up normal constructor calls:

compare: benchmark, old: aa95b1e39, new: 23cae4bcc
mode         | µs/iteration old | µs/iteration new | diff %      
creation     | 39.76            | 9.97             | -74.92      

compare: full_block, old: aa95b1e39, new: 23cae4bcc
mode         | µs/iteration old | µs/iteration new | diff %      
creation     | 53.07            | 10.26            | -80.66

Based on:

@xdustinface xdustinface requested review from altendky and mariano54 June 1, 2022 16:41
@xdustinface xdustinface mentioned this pull request Jun 1, 2022
Merged
@xdustinface xdustinface marked this pull request as draft June 2, 2022 08:48
@github-actions github-actions bot added the merge_conflict Branch has conflicts that prevent merge to main label Jun 2, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Jun 2, 2022

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

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.
@xdustinface xdustinface mentioned this pull request Jun 9, 2022
Merged
@xdustinface xdustinface force-pushed the pr-streamable-cache-post-init-processing branch 2 times, most recently from 9116d57 to a193b22 Compare June 10, 2022 09:44
@github-actions
Copy link
Contributor

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

@github-actions github-actions bot added merge_conflict Branch has conflicts that prevent merge to main and removed merge_conflict Branch has conflicts that prevent merge to main labels Jun 10, 2022
@github-actions
Copy link
Contributor

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

@xdustinface xdustinface force-pushed the pr-streamable-cache-post-init-processing branch from a193b22 to 1aa398e Compare June 14, 2022 17:58
@github-actions github-actions bot removed the merge_conflict Branch has conflicts that prevent merge to main label Jun 14, 2022
@github-actions
Copy link
Contributor

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

@xdustinface xdustinface force-pushed the pr-streamable-cache-post-init-processing branch from 1aa398e to 2bdd3e9 Compare June 14, 2022 18:48
@xdustinface xdustinface marked this pull request as ready for review June 14, 2022 18:48
@xdustinface xdustinface added the ready_to_merge Submitter and reviewers think this is ready label Jun 27, 2022
emlowe
emlowe approved these changes Jun 28, 2022
View reviewed changes
Copy link
Contributor

@emlowe emlowe left a comment

Choose a reason for hiding this comment

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

Are there streamable CI tests that would find regressions in here?

@wallentx wallentx merged commit 82a83b7 into Chia-Network:main Jun 28, 2022
@xdustinface
Copy link
Contributor Author

Are there streamable CI tests that would find regressions in here?

Yes there are __post_init__ tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@emlowe emlowe emlowe approved these changes

@altendky altendky altendky approved these changes

@mariano54 mariano54 Awaiting requested review from mariano54

@trepca trepca Awaiting requested review from trepca

@wjblanke wjblanke Awaiting requested review from wjblanke

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 @altendky @emlowe @wallentx