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

bug(dot/state): delete previous previous finalised trie from BlockState.tries #2341

Merged
merged 16 commits into from
Mar 8, 2022

Conversation

timwu20
Copy link
Contributor

@timwu20 timwu20 commented Mar 2, 2022

Changes

  • Deletes the previous trie after a block is finalised from BlockState.tries.
  • Significantly reduces memory usage over time
  • Added a tries set/delete gauge
  • Removed gauge mocks from dot/state tests.

Tests

go test ./dot/state

Issues

Primary Reviewer

@codecov
Copy link

codecov bot commented Mar 2, 2022

Codecov Report

Merging #2341 (cca78fd) into development (625cbcf) will increase coverage by 0.47%.
The diff coverage is 86.95%.

Impacted file tree graph

@@               Coverage Diff               @@
##           development    #2341      +/-   ##
===============================================
+ Coverage        57.55%   58.03%   +0.47%     
===============================================
  Files              212      213       +1     
  Lines            27952    28018      +66     
===============================================
+ Hits             16089    16259     +170     
+ Misses           10228    10118     -110     
- Partials          1635     1641       +6     
Flag Coverage Δ
unit-tests 58.03% <86.95%> (+0.47%) ⬆️

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

Impacted Files Coverage Δ
dot/state/test_helpers.go 49.76% <ø> (-0.24%) ⬇️
dot/state/block_finalisation.go 47.25% <83.33%> (+2.60%) ⬆️
dot/state/tries.go 100.00% <100.00%> (+7.31%) ⬆️
lib/babe/crypto.go 59.43% <0.00%> (-24.57%) ⬇️
dot/network/message.go 66.52% <0.00%> (-2.61%) ⬇️
dot/state/block.go 42.45% <0.00%> (-2.48%) ⬇️
dot/sync/chain_processor.go 48.04% <0.00%> (-1.67%) ⬇️
dot/network/light.go 85.25% <0.00%> (-0.80%) ⬇️
lib/babe/babe.go 3.58% <0.00%> (-0.02%) ⬇️
... and 12 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 625cbcf...cca78fd. Read the comment docs.

@timwu20 timwu20 marked this pull request as ready for review March 2, 2022 21:23
Copy link
Contributor

@qdm12 qdm12 left a comment

Choose a reason for hiding this comment

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

Nice, although all the metrics testing removal hurts my soul, can you please adapt the test code to take care of the new metrics added?

Also please remove the tries gauge in favor of the new set and delete counter metrics

dot/state/block_finalisation.go Show resolved Hide resolved
dot/state/block_race_test.go Outdated Show resolved Hide resolved
dot/state/block_test.go Show resolved Hide resolved
dot/state/tries.go Show resolved Hide resolved
dot/state/tries_test.go Outdated Show resolved Hide resolved
tests/stress/stress_test.go Outdated Show resolved Hide resolved
@timwu20 timwu20 force-pushed the tim/delete-prev-finalised-trie branch from d14e80e to 69c7e9e Compare March 4, 2022 02:52
Co-authored-by: Quentin McGaw <quentin.mcgaw@gmail.com>
@timwu20 timwu20 requested a review from qdm12 March 4, 2022 02:59
dot/state/block_finalisation.go Show resolved Hide resolved
dot/state/tries_test.go Outdated Show resolved Hide resolved
dot/state/tries_test.go Outdated Show resolved Hide resolved
dot/state/tries_test.go Show resolved Hide resolved
dot/state/block_test.go Show resolved Hide resolved
dot/state/block_test.go Show resolved Hide resolved
timwu20 and others added 3 commits March 4, 2022 14:11
Co-authored-by: Quentin McGaw <quentin.mcgaw@gmail.com>
Co-authored-by: Quentin McGaw <quentin.mcgaw@gmail.com>
@timwu20 timwu20 requested a review from qdm12 March 4, 2022 19:21
Copy link
Contributor

@noot noot left a comment

Choose a reason for hiding this comment

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

nice!

@timwu20 timwu20 force-pushed the tim/delete-prev-finalised-trie branch from bb85516 to cca78fd Compare March 5, 2022 03:38
@qdm12
Copy link
Contributor

qdm12 commented Mar 7, 2022

@noot can you merge this please? Thanks!

@qdm12 qdm12 merged commit 9ea0c65 into development Mar 8, 2022
@qdm12 qdm12 deleted the tim/delete-prev-finalised-trie branch March 8, 2022 16:54
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.

Delete previously finalised trie from BlockState.tries
3 participants