blockchain: remember topo Count() instead of re-walking the cleaned tail - #77
Open
DHEBP wants to merge 6 commits into
Open
blockchain: remember topo Count() instead of re-walking the cleaned tail#77DHEBP wants to merge 6 commits into
DHEBP wants to merge 6 commits into
Conversation
…ned tail Rewind_Chain zeroes popped topo records but never shortens topo.map, so Count() walks the clean tail one pread per popped record. derod calls it about fourteen times per connected block, which makes catching up after a deep pop unusable. The walk is unchanged. Its result is remembered and kept in step by Write(), so it runs once per invalidation instead of once per call. Count() therefore keeps returning exactly what the walk returns for every possible file state, including one with interior clean records.
The count_mu comment stated only that a reader never delays a writer. Write holds the mutex across its WriteAt, so the other direction is also true and worth saying.
Adds a test for the case the change exists to serve: while catching up a node only appends live records, which may raise the remembered count but must never invalidate it. Driven with the file closed after priming, so a walk would fail rather than quietly succeed. Also asserts directly that a failed write invalidates. A total failure leaves the file untouched, so the observable count cannot distinguish remembering from re-walking; a partial write grows the file and would leave the remembered value stale-low, so the branch still has to fire.
The comment asserted that these calls are what makes catching up after a deep pop unusable. That end-to-end effect was never measured. It also said ten calls per block where the measured figure is 13.80.
Rewind_Chain issues one Clean() per popped record, and each one invalidated the remembered count, so a concurrent Count() walked the growing clean run and then discarded the walk when the next Clean() bumped the generation. The memo helped only once the rewind was over. Where the chain will land is known before the first record is cleaned, so Rewind_Chain now says so up front, once, after confirming the record below that point is live. Write() no longer invalidates for a clean record written at or above a count already held: by the walk's own invariant that region is clean already, so the write changes nothing the walk would return. A clean record still never raises the count, and one written below the count still invalidates. Measured on a 60,000 record topo.map with four readers calling Count() throughout a 50,000 record rewind, three runs each: worst case per call 35-54 ms before, 3.0-3.5 ms after. p50 and p99 are unchanged at 0 and 2 us, so this is a tail fix, not a throughput one. The residual few milliseconds is waiting on the writer's own lock, not walking. Rewind_Chain itself is not faster.
Rewind_Chain computed the landing count and checked the record below it inline, where no test could reach either. Two deliberate errors in that arithmetic and a removed guard all went undetected. SettleForClean carries both, so the same shapes are covered by unit tests against a real storetopofs: the landing count agrees with a fresh walk at five depths, and the guard refuses a zero-length run, a run reaching index 0, a negative landing count, and a clean record directly below the landing point. Rewind_Chain keeps one call and the re-assert after the loop. Behaviour is unchanged.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
blockchain: remember the topo count instead of re-walking the cleaned tail
Rewind_Chain zeroes the popped records in topo.map but never shortens the file,
so Count() answers "how tall is the chain?" by walking that run of zeroed
records, one pread each, on every call - and derod calls it about fourteen times
per connected block (13.80, measured over 30 blocks on a simulator chain). This
keeps the walk's result instead of re-running it. The walk is unchanged byte for
byte; a clean record written at or above the count already held leaves the memo
alone, since it changes nothing the walk would return, and that is what carries
it through Rewind_Chain's run of Clean() calls. Measured in tmpfs, so a real
data directory can only be slower: with 400,000 popped, ~200 ms per Count()
before and ~11 ns after; with four readers running throughout a 50,000 record
rewind, worst case per call 35-54 ms before and 3.0-3.5 ms after over three
runs. Rewind_Chain itself is no faster, only the catch-up after it, and no real
node has been stopwatched recovering from a deep pop.