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 Ledger.Get() by making Forest.Read() use ~5x fewer allocs/op and run faster #2476

Closed
wants to merge 3 commits into from

Conversation

fxamacker
Copy link
Member

@fxamacker fxamacker commented May 24, 2022

Changes

Change Forest.Read() to return []ledger.Value without deep copying payload keys. This avoids 4 heap allocation per key.

This change doesn't affect Ledger.Get() (the caller) because it discards the payload keys.

Benchmark comparison for batch read size of 100 keys

name        old time/op    new time/op    delta
TrieRead100-4     524µs ± 1%     420µs ± 1%  -19.77%  (p=0.000 n=10+10)

name        old alloc/op   new alloc/op   delta
TrieRead100-4     190kB ± 0%      95kB ± 0%  -50.04%  (p=0.000 n=10+10)

name        old allocs/op  new allocs/op  delta
TrieRead100-4     1.52k ± 0%     0.32k ± 0%  -79.17%  (p=0.000 n=10+10)

Closes #2475

Change Forest.Read to return []ledger.Value without deep copying
payload keys.  This avoids 4 heap allocation per key.

This change doesn't affect the caller (Ledger.Get) because it
discards the payload keys.

name        old time/op    new time/op    delta
TrieRead-4     524µs ± 1%     420µs ± 1%  -19.77%  (p=0.000 n=10+10)

name        old alloc/op   new alloc/op   delta
TrieRead-4     190kB ± 0%      95kB ± 0%  -50.04%  (p=0.000 n=10+10)

name        old allocs/op  new allocs/op  delta
TrieRead-4     1.52k ± 0%     0.32k ± 0%  -79.17%  (p=0.000 n=10+10)
@codecov-commenter
Copy link

Codecov Report

Merging #2476 (f026cc3) into master (1d6be74) will increase coverage by 0.03%.
The diff coverage is 83.33%.

@@            Coverage Diff             @@
##           master    #2476      +/-   ##
==========================================
+ Coverage   56.69%   56.72%   +0.03%     
==========================================
  Files         656      656              
  Lines       60175    60217      +42     
==========================================
+ Hits        34114    34157      +43     
+ Misses      23123    23122       -1     
  Partials     2938     2938              
Flag Coverage Δ
unittests 56.72% <83.33%> (+0.03%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
ledger/complete/mtrie/forest.go 62.96% <80.00%> (ø)
ledger/complete/ledger.go 60.77% <100.00%> (+0.45%) ⬆️
...s/hotstuff/votecollector/staking_vote_processor.go 83.87% <0.00%> (-3.23%) ⬇️
insecure/wintermute/helpers.go 96.64% <0.00%> (+0.07%) ⬆️
engine/execution/ingestion/engine.go 52.54% <0.00%> (+0.28%) ⬆️
insecure/wintermute/attackOrchestrator.go 72.39% <0.00%> (+0.50%) ⬆️
engine/verification/verifier/engine.go 51.55% <0.00%> (+0.76%) ⬆️
insecure/corruptible/factory.go 62.80% <0.00%> (+4.93%) ⬆️
module/mempool/epochs/transactions.go 100.00% <0.00%> (+9.67%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1d6be74...f026cc3. Read the comment docs.

@fxamacker
Copy link
Member Author

Closing this because it was based on master. I'll open new PR based on PR #2473.

@fxamacker fxamacker closed this May 25, 2022
@fxamacker fxamacker deleted the fxamacker/optimize-ledger-read-allocs branch May 25, 2022 00:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Execution] Forest.Read() can use less memory
2 participants