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 get_peaks_and_heights for archival MMR. #141

Merged
merged 3 commits into from
Apr 26, 2024

Conversation

zkclay
Copy link
Contributor

@zkclay zkclay commented Apr 23, 2024

This PR is a response to Issue #125

I chose to keep the get_peaks_and_heights_async and place the new functionality inside it rather than in get_peaks because the async function is used 5 or 6 times within the test suite. I ran the benchmarks before and after the change and obtained the following results:

Master:

before_change

My PR:

after_change

In multiple runs I noticed the same small but consistent improvement on my machine, its possible that in setups with different tradeoffs between disk I/O and CPU speed, the difference would be more pronounced.

EDIT:

I definitely see even more gains after removing the get_peaks_and_heights_async function completely

deeper_cut

@dan-da
Copy link
Collaborator

dan-da commented Apr 24, 2024

Thanks @zkclay for the PR. I like the comparative simplicity of this approach.

lgtm, but i will defer to @Sword-Smith on this one.

@Sword-Smith
Copy link
Member

Sword-Smith commented Apr 24, 2024

Let's get rid of get_peaks_and_heights_async. All calls to it in test can be replaced by a call to get_peaks, or be deleted entirely.

This will probably also make your benchmark even faster :)

@zkclay
Copy link
Contributor Author

zkclay commented Apr 25, 2024

@Sword-Smith I have gotten rid of the function completely and added the updated benchmark above. I think removing it completely was a good ideas, aside from the speedup, it's satisfying to delete even more code 😄

@Sword-Smith Sword-Smith self-requested a review April 26, 2024 05:05
Copy link
Member

@Sword-Smith Sword-Smith left a comment

Choose a reason for hiding this comment

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

Very nice :) Glad to see a 20 % speedup. LGTM.

Edit: We're intending to do the same for the ArchivalMutatorSet btw. In case you're looking for more stuff to do :)

@Sword-Smith Sword-Smith merged commit 9a79732 into Neptune-Crypto:master Apr 26, 2024
3 checks passed
@zkclay
Copy link
Contributor Author

zkclay commented Apr 27, 2024

Thanks, @Sword-Smith1 I will get to work on it! I assume that's the work covered by Issue #127 ?

@Sword-Smith
Copy link
Member

Thanks, @Sword-Smith1 I will get to work on it! I assume that's the work covered by Issue #127 ?

It is :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants