search

Neptune-Crypto / neptune-core

anonymous peer-to-peer cash
Apache License 2.0
25 stars 7 forks source link

Optimize get_peaks_and_heights for archival MMR. #141

Closed zkclay closed 5 months ago

zkclay commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

@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 😄

zkclay commented 5 months ago

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

Sword-Smith commented 5 months ago

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

It is :)