flow-go icon indicating copy to clipboard operation
flow-go copied to clipboard

Remove duplicated top-level `EncodableSnapshot` fields

Open jordanschalm opened this issue 9 months ago • 1 comments

This PR removes the Head, LatestSeal, and LatestResult fields from EncodableSnapshot, addressing this outstanding TODO from https://github.com/onflow/flow-go/issues/5666:

Remove additional redundant fields from EncodableSnapshot (in particular LatestSeal, Result, Head). See https://github.com/onflow/flow-go/pull/5682#issuecomment-2080002868

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.

jordanschalm avatar May 09 '24 19:05 jordanschalm

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 :warning:
state/protocol/inmem/snapshot.go 25.00% 7 Missing and 2 partials :warning:
state/protocol/badger/validity.go 75.00% 1 Missing :warning:
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%) :arrow_up:

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

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov-commenter avatar May 09 '24 19:05 codecov-commenter