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.

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

optimize launcher_id_to_p2_puzzle_hash() #17961

Merged
merged 1 commit into from
May 7, 2024
Merged

Conversation

arvidn
Copy link
Contributor

@arvidn arvidn commented May 3, 2024

Purpose:

the wallet spends a fair amount of time generating these puzzle hashes repeatedly.

function before after after/before
launcher_id_to_p2_puzzle_hash() 32.48% 1.47% 0.0453

Current Behavior:

launcher_id_to_p2_puzzle_hash() generates the full puzzle, just to tree-hash it.

New Behavior:

launcher_id_to_p2_puzzle_hash() generates the puzzle hash directly.

profile before:

chia-hotspot-375

profile after:

chia-hotspot-36

@arvidn arvidn added the Changed Required label for PR that categorizes merge commit message as "Changed" for changelog label May 3, 2024
@arvidn arvidn requested a review from richardkiss May 3, 2024 09:47
@arvidn arvidn marked this pull request as ready for review May 3, 2024 10:36
@arvidn arvidn requested a review from a team as a code owner May 3, 2024 10:36
@arvidn arvidn force-pushed the optimize-plotnft-puzzle-hash branch from e375fcb to 012c839 Compare May 3, 2024 10:58
Copy link

Pull Request Test Coverage Report for Build 8937938907

Details

  • 6 of 6 (100.0%) changed or added relevant lines in 1 file are covered.
  • 44 unchanged lines in 11 files lost coverage.
  • Overall coverage decreased (-0.03%) to 90.659%

Files with Coverage Reduction New Missed Lines %
chia/daemon/keychain_proxy.py 1 64.98%
chia/harvester/harvester_api.py 1 73.02%
chia/full_node/full_node_api.py 1 80.92%
chia/farmer/farmer.py 1 72.56%
chia/wallet/util/wallet_sync_utils.py 1 86.54%
chia/daemon/client.py 1 73.33%
chia/data_layer/data_layer.py 2 85.26%
chia/server/node_discovery.py 4 79.96%
chia/wallet/wallet_node.py 5 88.39%
chia/full_node/full_node.py 5 85.19%
Totals Coverage Status
Change from base Build 8931619722: -0.03%
Covered Lines: 97981
Relevant Lines: 108020

💛 - Coveralls

@richardkiss
Copy link
Contributor

richardkiss commented May 3, 2024

Seems reasonable.

The nice thing about this sort of optimization is if it produces the correct value once, it almost certainly will work every time.

A trick that I like to do for changes like this is to calculate the value both the old and new way and assert they are the same. Then run the test suite (or run it on a wallet with a lot of these addresses). If you do this, there's a pretty high confidence it works correctly.

@arvidn arvidn force-pushed the optimize-plotnft-puzzle-hash branch from 012c839 to a3cb749 Compare May 4, 2024 16:30
@arvidn arvidn force-pushed the optimize-plotnft-puzzle-hash branch from a3cb749 to 1ced693 Compare May 4, 2024 16:34
@arvidn arvidn requested a review from richardkiss May 4, 2024 19:56
Copy link
Contributor

@richardkiss richardkiss left a comment

Choose a reason for hiding this comment

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

Seems reasonable.

@arvidn arvidn requested a review from wjblanke May 6, 2024 22:04
Copy link
Contributor

@wjblanke wjblanke left a comment

Choose a reason for hiding this comment

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

aok

@arvidn arvidn added the ready_to_merge Submitter and reviewers think this is ready label May 7, 2024
@pmaslana pmaslana merged commit 35851e5 into main May 7, 2024
358 checks passed
@pmaslana pmaslana deleted the optimize-plotnft-puzzle-hash branch May 7, 2024 20:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changed Required label for PR that categorizes merge commit message as "Changed" for changelog ready_to_merge Submitter and reviewers think this is ready
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants