common/math, cl, polygon/bor, execution/cache: deduplicate NextPowerOfTwo - #22476
Merged
Conversation
…counting exact Split from #22159 (StateCache review findings #22120). - maybeGrow copies and swaps the generation with every put stripe held, so a striped put can no longer land in a retired generation (older value resurfacing as live) and a conditional put can no longer fill a mid-resize gap with a stale value. Grow detection moves inside the stripe, the grow itself outside it; the triggering insert (and racers until the swap) evict at the pre-grow cap — a bounded transient. - Clear performs its counter reset, coherence re-init and generation swap under the same fence. - Delete and the lazy stale-drop run under the key's put stripe, and currentSize is subtracted solely via the OnEvict callback: freelru picks eviction victims per shard (hash bits 16+), which the stripes (bits 0-7) don't cover, so any subtraction computed outside the callback double-counts against a racing capacity eviction. - One Debug line per grow (caps, copied count, alloc/fenced split) so the once-per-lifetime writer stall is self-explaining in logs. - growLRU documents its intentionally unfenced swap contract: safe only for content-addressed layers; counters approximate across grows.
putLocked read coh.Epoch() before acquiring the key's stripe, so a put losing the stripe race to Clear (whose coh.Init resets the epoch counter) stamped a pre-Clear epoch onto an entry landing in the post-Clear generation. Once a later unwind re-reached that epoch value, the entry aliased the live epoch and survived IsStale despite its txNum being at or above the floor - serving dead-fork state. Move the epoch sample inside the stripe, next to the generation load the fence already synchronizes. The new test parks Clear on the key's stripe pre-Init and the put behind it (starvation-mode FIFO hands the stripe to Clear first); it failed deterministically before the fix.
…ert window The prober only burst conditional puts after observing the generation swap, but every design publishes the new generation after the copy completes, so the hot key was always present and the test could not fail - with or without the fence. Replace the body: a writer hammers puts of brand-new keys while an insert at a lowered curCap triggers the grow, then every key that straddled the swap is probed with a stale PutIfAbsent. Unfenced, a new-key put landing in the retiring generation after the Keys() snapshot is lost on the swap and the conditional put installs the stale value as live - the test now fails in the first rounds when the fence is removed (20/20 runs). The candidate presence asserts also give a stronger migration-completeness check than the old single hot key.
removeLocked compensated the OnEvict increment by decrementing the evictions counter; a PrintStatsAndReset Swap(0) landing between the two zeroed the counter first, so the decrement underflowed it to ~1.8e19 for the next interval. The remove-then-add update path fires this on every warm update, so the window is open a measurable fraction of wall time on a busy node. Count intentional removals in their own counter instead - both counters are increment-only between resets, so no interleaving with a reset can underflow either - and net them at print time. Removals are snapshotted before evictions: OnEvict bumps evictions before the removal is counted, so this order keeps every captured removal paired with a captured eviction; the clamp absorbs a removal deferred to the next interval. Reported semantics are unchanged: evictions still means capacity evictions only. The new test plays the stats reset against a Delete hammer; it underflowed within milliseconds before the fix.
The -Locked suffix conventionally means the caller holds the lock, which is removeLocked's contract; putStriped acquires the stripe itself.
…movals dropStale's stripe-held re-check keeps an entry a racing put revived, so the counter tracks detections; the old comment implied actual drops.
OnEvict fires for intentional Removes too, so any scheme that routes them through the evictions counter - decrement compensation or netting against a removals counter at print time - races a concurrent stats reset: the swap straddles the paired updates and reports phantom evictions (or underflows). Count evictions where they happen instead, from freelru.Add's evicted return, and leave OnEvict with size accounting only. This deletes the removals counter and the print-time netting, and surfaces copy evictions in the jump-grow log line.
Left to pick its own geometry per generation, freelru chooses more, smaller shards as capacity rises (it derives the count from GOMAXPROCS and table size), so a new shard receiving more entries than its capacity evicts during the migration copy - keys clustered on the shard-selection bits silently vanish across a grow, and a follow-up conditional put can install a stale value in the hole. Generations now get an explicit shard count: a lineage starts at ~64 entries per shard and the count doubles across grows only while per-shard capacity does not shrink, bounded by freelru's own GOMAXPROCS-derived ceiling. Power-of-two counts make the selection bits nest, so each new shard receives a subset of exactly one old shard and the copy can never overfill one. Steady-state shard counts at full size match what freelru would choose; the jump-grow log line now also reports the shard count.
The update and collision paths removed the old entry before adding the new one and only then adjusted the byte counter, so a concurrent ModeNoOp admission on another stripe could observe the transient dip and admit a key past a full budget - breaking "drop new keys when full". Reserve the new size before the removal instead: the counter transiently over-states usage, which at worst drops a new key, and the settled sums are unchanged.
…fTwo Three identical implementations (cl/merkle_tree, polygon/bor, and a private one in execution/cache) consolidate into common/math, the one shared leaf package all three can import. Behavior-preserving, including the n == 0 -> 1 and above-1<<63 -> 0 edges; a unit test pins the shared contract.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR deduplicates the repository’s multiple NextPowerOfTwo helpers by consolidating them into common/math (a shared leaf package), then updates all call sites to use the shared implementation and adds a unit test to pin the contract.
Changes:
- Added
common/math.NextPowerOfTwo(uint64) uint64and a unit test covering key edge cases. - Removed the duplicated
NextPowerOfTwoimplementations fromcl/merkle_treeandpolygon/bor. - Updated call sites in CL merkleization code, Bor header-root computation, and
execution/cacheshard sizing to use the shared helper.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| polygon/bor/merkle.go | Removes local NextPowerOfTwo implementation. |
| polygon/bor/bor.go | Switches to common/math.NextPowerOfTwo (aliased as math2). |
| execution/cache/generic_cache.go | Replaces private nextPow2 with common/math.NextPowerOfTwo. |
| common/math/integer.go | Adds shared NextPowerOfTwo implementation. |
| common/math/integer_test.go | Adds unit test for NextPowerOfTwo edge cases. |
| cl/merkle_tree/utils.go | Removes duplicated NextPowerOfTwo. |
| cl/merkle_tree/primitives.go | Updates to common/math.NextPowerOfTwo. |
| cl/merkle_tree/merkle_root.go | Updates to common/math.NextPowerOfTwo. |
| cl/cltypes/solid/vector.go | Updates vector merkleization limit calc to common/math.NextPowerOfTwo. |
| cl/cltypes/solid/vector_test.go | Updates test to use common/math.NextPowerOfTwo. |
| cl/cltypes/solid/hash_vector.go | Updates capacity rounding to common/math.NextPowerOfTwo and drops unused import. |
| cl/cltypes/solid/byte_list.go | Updates leaf-count rounding to common/math.NextPowerOfTwo. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…of-two-dedup # Conflicts: # cl/merkle_tree/merkle_root.go # execution/cache/cache_test.go # execution/cache/code_cache_concurrency_test.go # execution/cache/generic_cache.go # execution/cache/generic_cache_concurrency_test.go # polygon/bor/bor.go
yperbasis
marked this pull request as ready for review
July 17, 2026 14:59
yperbasis
requested review from
AskAlexSharov,
domiwei,
mh0lt and
sudeepdino008
as code owners
July 17, 2026 14:59
yperbasis
enabled auto-merge
July 17, 2026 15:00
AskAlexSharov
approved these changes
Jul 20, 2026
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
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.
The tree carried three identical
NextPowerOfTwoimplementations:cl/merkle_tree/utils.go,polygon/bor/merkle.go(byte-for-byte copies of the Stanford bithack), and a privatenextPow2inexecution/cache/generic_cache.go(added by #22466 — neither existing copy is importable from execution code without inverting the layering). All three consolidate intocommon/math, the shared leaf package every call site can import.Behavior-preserving, including the edges:
0 → 1, exact powers unchanged, and inputs above1<<63wrap to 0 (as the bithack did).