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

(WIP) [EN Performance] Split Node into LeafNode and InterimNode #2265

Closed
wants to merge 2 commits into from

Conversation

fxamacker
Copy link
Member

@fxamacker fxamacker commented Apr 6, 2022

Changes

Split Node into LeafNode and InterimNode and remove unnecessary fields to reduce memory:

  • 16 bytes for each leaf node
  • 24 bytes for each interim node

Create Node interface with read-only functions for LeafNode and InterimNode.

Updates Epic #1744

Caveats

  • 🚧 WIP - Looking into why benchnet test shows only 4.4GB savings (expected more).
  • 🐢 Calling interface methods which cannot be inlined will have a performance cost.
  • ⚠️ Need to decide if the memory savings doesn't outweigh the performance cost.

Split Node into LeafNode and InterimNode and
remove unnecessary fields to reduce memory
by about 8.3GB (est. for 400 million nodes):
- 16 bytes for each leaf node
- 24 bytes for each interim node

Create Node interface with read-only functions for
LeafNode and InterimNode.
@codecov-commenter
Copy link

Codecov Report

Merging #2265 (7ed3b47) into master (ba1f157) will decrease coverage by 0.08%.
The diff coverage is 48.72%.

@@            Coverage Diff             @@
##           master    #2265      +/-   ##
==========================================
- Coverage   57.47%   57.38%   -0.09%     
==========================================
  Files         645      651       +6     
  Lines       38428    39138     +710     
==========================================
+ Hits        22085    22458     +373     
- Misses      13530    13810     +280     
- Partials     2813     2870      +57     
Flag Coverage Δ
unittests 57.38% <48.72%> (-0.09%) ⬇️

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

Impacted Files Coverage Δ
cmd/bootstrap/transit/cmd/version.go 14.28% <ø> (ø)
cmd/util/ledger/reporters/export_reporter.go 0.00% <0.00%> (ø)
engine/common/rpc/convert/convert.go 16.07% <0.00%> (+2.13%) ⬆️
engine/execution/state/state.go 22.12% <0.00%> (-0.53%) ⬇️
storage/badger/commits.go 90.00% <0.00%> (-3.11%) ⬇️
storage/badger/events.go 53.24% <0.00%> (-1.42%) ⬇️
storage/badger/headers.go 44.33% <0.00%> (-9.07%) ⬇️
storage/badger/my_receipts.go 58.73% <0.00%> (-0.95%) ⬇️
storage/badger/operation/chunkDataPacks.go 75.00% <0.00%> (ø)
storage/badger/operation/commits.go 50.00% <0.00%> (-16.67%) ⬇️
... and 42 more

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 b341963...7ed3b47. Read the comment docs.

@fxamacker fxamacker added the Execution Cadence Execution Team label Jun 16, 2022
@fxamacker fxamacker changed the title (WIP) [Execution State] Split Node into LeafNode and InterimNode (WIP) [EN Performance] Split Node into LeafNode and InterimNode Aug 10, 2022
@Kay-Zee
Copy link
Member

Kay-Zee commented Nov 25, 2022

@fxamacker @ramtinms @m4ksio What's the status on this issue? Getting quite stale.

@fxamacker
Copy link
Member Author

@Kay-Zee This was paused because it sacrifices EN performance to gain memory (for both reads and updates). In August Spork, checkpoint v5 memory reductions (especially after PR 3050 got deployed to mainnet before October Spork) made it less important for EN to reduce memory by sacrificing performance.

I can close this for now and it can be reopened if needed.

Thoughts?

cc: @ramtinms

@fxamacker
Copy link
Member Author

Confirmed with @ramtinms that this can be closed for now.

@fxamacker fxamacker closed this Dec 2, 2022
@fxamacker fxamacker deleted the fxamacker/separate-node-type branch February 2, 2024 22:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Execution Cadence Execution Team Performance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants