Substrate: give PoolAllocator a FIFO free list - #113
Open
PowerOfNames wants to merge 1 commit into
Open
Conversation
Allocate popped m_FreeHandles.back() and Free pushed back, so a churning caller cycled a handful of indices and burned their generation counters to exhaustion while the rest of the pool sat untouched. Resize churn retired the first image slot after roughly 63 resizes. The free list is now a ring buffer over the same fixed-capacity index array -- head, tail and an explicit free count -- so reuse rotates through every slot and a freed handle stays detectably stale for far longer. MAX_BLOCK_COUNT is not always a power of two, so the wrap is a compare rather than a mask. BaseHandle: IsValid() no longer treats "all index bits set" as invalid. When the block count is an exact power of two the highest valid index equals the index mask, which made that slot allocatable but never freeable -- Free took the retirement branch immediately, so the index never returned to the list and the allocation count never decremented. Both Aurora pools are that shape at 1024 blocks. Validity now keys off the generation alone, which already subsumes the all-ones sentinel. A handle type must leave at least one index bit, enforced by static_assert instead of by every instance reading invalid. Slot retirement is observable: GetMaxedGenerationCount() on AllocatorBase, alongside GetCurrentAllocationCount(), which still counts retired slots. Tests: rewrote the two PoolAllocator cases that asserted LIFO reuse, moved generation exhaustion onto a small uint16_t pool so it runs in milliseconds rather than 33.5M iterations, and added coverage for rotation, burn spreading, ring wrap, retirement and the power-of-two top slot. Added the missing CreateRefFromThis cases, including the consumer shape that caught the original missing AddRef. Removed two TestAllocationHandle sections that instantiated DefineHandle<16, 0, uint16_t>, which the new static_assert rejects. 783 assertions in 29 cases, green. Sandbox verified in Debug and Release. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HGxv2j7K5MFmFMX1iKrgcq
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.
Closes #108.
Why
Allocatepoppedm_FreeHandles.back()andFreepushed back — LIFO, so a churning caller cycled a handful of indices and burned their generation counters to exhaustion while the rest of the pool sat untouched. Resize churn proved it: ~6 image slots absorbed everything and the first retired after about 63 resizes, roughly a second of dragging a window border.FIFO spreads the burn across every slot and keeps a freed handle detectably stale for far longer, which makes use-after-free easier to catch.
Shape
A ring buffer over the existing fixed-capacity
m_FreeHandles, plus head, tail and an explicitm_FreeCount. The count is what disambiguates full from empty — the ring is completely full at construction, sohead == tailis the normal state rather than an edge case.MAX_BLOCK_COUNTis not always a power of two (85 in the tests, 1024 in Aurora), so the wrap is a compare, never a mask.The perf/instrumentation harness originally scoped alongside this was dropped: a microbenchmark on a fixed-size pool would mostly measure the branch predictor. LIFO's tail-only access only pays when allocate and free happen in close succession; across any real gap the cache holds other work anyway, and the ring stays contiguous regardless.
Bug fixed along the way
BaseHandle::IsValid()treated "all index bits set" as invalid. When the block count is an exact power of two the highest valid index equals the index mask, so that slot was allocatable but never freeable —Freetook the retirement branch immediately, the index never returned to the free list, andm_CurrentAllocationCountnever decremented. Both Aurora pools are that shape at 1024 blocks, and the FIFO change promotes this from latent to reachable: under LIFO the top index only came out at a completely full pool, but the ring head sweeps every index regardless of how many slots are live.Validity now keys off the generation alone, which already subsumes the all-ones sentinel. A handle type must leave at least one index bit, enforced by
static_assertrather than by every instance reading invalid.Also
GetMaxedGenerationCount()onAllocatorBase. Read alongsideGetCurrentAllocationCount(), which still counts retired slots and so overstates live allocations once any retire.GetFreeHandleIndices()removed — the backing vector is full-length forever now, so its.size()no longer means anything.Tests
783 assertions in 29 cases, green. Sandbox verified in Debug and Release.
PoolAllocatorsections that asserted LIFO reuse.uint16_tpool: 16383 generations instead of 33.5M, so it runs in milliseconds.IsValidbug above.CreateRefFromThiscoverage, including the consumer shape that caught the original missingAddRef— a scope-local object copying the returnedRefinto a member.TestAllocationHandlesections that instantiatedDefineHandle<16, 0, uint16_t>. They pinned the old runtime behaviour for an all-generation mask, which is now a compile error; the contract moved from runtime to compile time.Follow-ups, not in this PR
Freestill has no already-retired guard, soFree(INVALID_HANDLE)after the top slot retires inflatesGetMaxedGenerationCount(). One-line fix, same shape as the guard now inIsHandleValid.PoolAllocator'sMustBePowerOFTwoconstrains nothing and cannot be repaired as written — asserting onTSizerejects Aurora's pools, asserting onMAX_BLOCK_COUNTrejects the test pool. Needs a decision about what it was protecting.🤖 Generated with Claude Code
https://claude.ai/code/session_01HGxv2j7K5MFmFMX1iKrgcq