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

Remove duplicated top-level EncodableSnapshot fields #5884

Merged
merged 15 commits into from
May 20, 2024

Conversation

jordanschalm
Copy link
Member

@jordanschalm jordanschalm commented May 9, 2024

This PR removes the Head, LatestSeal, and LatestResult fields from EncodableSnapshot, addressing this outstanding TODO from #5666:

Remove additional redundant fields from EncodableSnapshot (in particular LatestSeal, Result, Head). See #5682 (comment)

In this implementation, I removed the redundant fields and replaced them with getters which searched for the corresponding data in the SealingSegment.

  • For Head, it's very fast and easy to find the data in SealingSegment
  • For LatestSeal, it's relatively fast and a bit more conceptually complex to find the data. In the worst case we might need to search through one non-empty block payload, and hash several seals.
  • For LatestResult, it's relatively easy, but potentially slow to find the data. In the worst case we might search through many block payloads and hash many results.

These methods are used sparingly on inmem.Snapshot (everything in the critical path uses badger.Snapshot, so I don't think the performance regression will have a meaningful impact.

@codecov-commenter
Copy link

codecov-commenter commented May 9, 2024

Codecov Report

Attention: Patch coverage is 45.45455% with 24 lines in your changes are missing coverage. Please review.

Project coverage is 55.79%. Comparing base (0436edf) to head (d9fc06c).

Files Patch % Lines
state/protocol/inmem/encodable.go 50.00% 12 Missing and 2 partials ⚠️
state/protocol/inmem/snapshot.go 25.00% 7 Missing and 2 partials ⚠️
state/protocol/badger/validity.go 75.00% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##           master    #5884   +/-   ##
=======================================
  Coverage   55.79%   55.79%           
=======================================
  Files        1128     1128           
  Lines       88998    89021   +23     
=======================================
+ Hits        49652    49665   +13     
- Misses      34608    34616    +8     
- Partials     4738     4740    +2     
Flag Coverage Δ
unittests 55.79% <45.45%> (+<0.01%) ⬆️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jordanschalm jordanschalm requested review from durkmurder, AlexHentschel and kc1116 and removed request for durkmurder May 9, 2024 19:58
@jordanschalm jordanschalm marked this pull request as ready for review May 15, 2024 14:51
Copy link
Member Author

Choose a reason for hiding this comment

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

Contained only unused types

Copy link
Member

@durkmurder durkmurder left a comment

Choose a reason for hiding this comment

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

Nice refactoring, like the simplification a lot. I think performance is not the issue in this case. The only thing I would tweak is traversing in reverse order in LatestSealedResult since we already do that for latest seal.

Copy link
Member

@AlexHentschel AlexHentschel 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 cleanup ✨. Thank you. Added some minor suggestion to reduce some minor ambiguity in the code. Feel free to skip, it if turns into a lot of effort. Not super important, but I think nice to have. Thanks

state/protocol/inmem/encodable.go Outdated Show resolved Hide resolved
state/protocol/inmem/encodable.go Outdated Show resolved Hide resolved
state/protocol/inmem/encodable.go Show resolved Hide resolved
consensus/integration/epoch_test.go Outdated Show resolved Hide resolved
@jordanschalm jordanschalm added this pull request to the merge queue May 20, 2024
Merged via the queue into master with commit 55fd3bb May 20, 2024
55 checks passed
@jordanschalm jordanschalm deleted the jordan/rm-redundant-sealing-segment-fields branch May 20, 2024 16:47
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.

4 participants