Stop every pooled rent allocating - #695
Merged
Eli Pinkerton (wallstop) merged 12 commits intoSep 3, 2026
Merged
Conversation
A warm pooled rent was not allocation-free, which is the claim the whole buffer layer rests on. PoolUsageTracker records one sample per rent into a CyclicBuffer whose backing List starts at capacity zero and doubles up to 10,000 entries, so every pool allocated a new Sample[] on rents 1, 3, 5, 9, 17 ... 8193 and then held 131 KB of samples for the process. Measured in the editor 6000.4.6f1 on a fresh pool: twelve growth events in 3,000 rents, at exactly those indices. That is #693's split, too. QuadTree2D and RTree2D each have their own node type and therefore their own pool, so each paid its boundary crossings inside its own fixture; both KdTree2D fixtures construct KdTree2D<Vector2> and share one pool, so the balanced fixture paid them and the unbalanced one ran past. EditMode passed because the editor domain had already warmed those pools. The tracker now keeps a fixed ring of 64 time buckets sized once at construction: recording is O(1), never allocates, and costs 2 KB per pool rather than 131 KB. Peak and average stay exact over the samples in the window; only expiry becomes bucket-granular, and it drops late rather than early. The two tests that encoded the old count cap ("the first 5000 samples are evicted") asserted the storage artifact rather than the window, and say what the window means now. #691, TextureScale: a background slice's exception was swallowed by the finally that signals the countdown, so the wait returned normally and the caller got a texture with a band of the pooled buffer's previous contents and no log. The first slice failure is captured and rethrown after the wait, which is what the single-threaded branch already did with the same failure. Sweep: the two other Task.Run sites in the package both observe their tasks. #644, the last recorded remainder: the four relational FieldsByType caches were plain Dictionary with a check-then-act GetOrAdd and no #if, beside ConcurrentDictionary neighbours. They now follow ReflectionHelpers' pattern. #637: CyclicBuffer was the one collection whose deserialized capacity was still honored verbatim, on all three paths. Refused rather than clamped, as SparseSet is -- capacity decides where a ring starts overwriting, so shrinking it silently changes behavior. And ImmutableBitSet's surrogate could build a value whose capacity exceeded its words, which made TryGet and All throw from a value the caller was handed successfully; capacity is now bounded by the words delivered. Refs #693, #691, #644, #643, #642, #637 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The package already had a gate on "a pooled rent allocates nothing", and its warm-up is exactly what hid the defect: RentAndReturnAllocatesNothing warmed 10,000 iterations, and the usage tracker's last growth boundary is at 8,193. The measurement began after the ramp was over. 256 leaves six boundaries inside the measured window, so the test now fails on the ramp as well as on the steady state, and the comment recording "280 bytes for the first ten cycles" -- which was that ramp -- says what it actually was. That makes the separate fixture added in the previous commit redundant, so it is gone rather than left as a second instrument for one property. #635: the ten Tests/Runtime/Pool files leave the baseline, 1247 runs across 272 files down to 1144 across 262. Deleted where the run restated the code (seven "should clean up the dead WeakReference entry and not throw" runs that repeat their own test names, sixteen ==== banner blocks), collapsed to one line where the why fits on one, and converted to a block where it does not. #640: DestroyCosmeticInstance read CleansUpSelf and data.gameObject with no destroyed check, the last unowned finding in that issue's audit. A component that cleans itself up can take its neighbors with it, and Destroy runs OnDestroy immediately outside play mode; RequiresInstancing and GetCurrentCosmeticTypes already check. Refs #693, #635, #640 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Helpers.Find(tag) parks the component it found in a process-lifetime static Dictionary<string, UnityEngine.Object>. The only eviction is the next lookup of the same tag noticing the object is destroyed -- which is exactly the lookup that never comes for a tag one level used. Until then the entry roots the MonoBehaviour, and everything its managed fields reach, for the process. Same class as the comparer-keyed pool caches in #689, with a bigger object on the wrong end of the reference. An unloaded scene now drops the entries whose objects went with it, leaving anything that survived the unload cached; ClearTagCache() drops the lot. The hook re-subscribes at SubsystemRegistration rather than guarding with a static latch, because a session with Domain Reload disabled runs it again over live statics. The documented example was also calling signatures that do not exist -- ClearInstance<Player>() and SetInstance(playerInstance) both take a tag -- and claiming the cache persists until manually cleared. Refs #643 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Re-deriving the ring arithmetic found two holes the rewrite left. A record whose epoch is older than the window has an index that aliases a slot a LIVE epoch already owns -- slots are epoch mod 66 and live epochs span at most 66 -- so it would have replaced that bucket's contribution to _runningSum and _sampleCount without subtracting what it displaced. It is outside the window, so it is dropped. Only an out-of-order clock produces one; Time.realtimeSinceStartup does not, but a caller's does, and the PoolUsageTracker fixtures already record out of order. And oldestLiveEpoch = epoch - 65 underflows for an epoch near long.MinValue, which EpochFor can return for a sufficiently negative time. Saturating. The test pins the aliasing case specifically: epochs 660 and 594 are 66 apart, so they are the same slot, and both timestamps are exact in float. Refs #693 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
**A main-thread slice failure discarded a worker's.** The on-thread slice threw through the finally and never reached the rethrow, so a worker failure captured moments earlier was lost -- worse than before, where it at least surfaced as an unobserved Task exception. Both paths now record into the same field, first failure wins, and the loser is logged rather than dropped. **The slice-failure test could not fail on a one-core runner.** cores is Min(processorCount, newHeight), so every case would have taken the single-threaded branch and passed without a background slice ever running -- a green report for the one shape the test exists to cover. It ignores when the runner cannot reach the parallel branch, and asserts the height-1 rows land on the single-threaded one. **A denormal window made EpochFor convert NaN to long.** WindowSeconds accepts any positive float; 1e-44f divides to a bucket duration of zero, and EpochFor(0f) is then 0f/0f. The cast is undefined under IL2CPP rather than merely wrong. **The JSON CyclicBuffer path changed behaviour, not just bounds.** TryAccept uses the delivered count as a FLOOR, so a document with capacity 2 and five items now keeps five where it used to keep the last two. That matches what both binary paths already did, so it stays -- and the comment and CHANGELOG say so instead of claiming nothing moved. **A rewritten comment misstated the code.** GlobalPoolRegistry.Unregister removes the entry synchronously under its lock, so "the count catches up on the next cleanup" was false. Deleted, per the rule that prefers deletion. CHANGELOG: the CyclicBuffer limit folds into the unreleased Security entry that already lists the other four collections rather than being a Fixed entry for a feature that never shipped, and the two behaviour changes -- the averaging window and the JSON capacity floor -- are recorded. Refs #693, #691, #637 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The JSON comment now names the floor TryAccept applies; the two binary comments described only the ceiling, so a reader comparing them would have concluded the three paths differ. They do not. Refs #637 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This was referenced Sep 3, 2026
Open
Two of the first review's fixes were incomplete, and both are the same mistake one level out. **The Task.Run throw re-opened the defect I had just closed.** The rethrow sits after the try/finally, so an exception escaping the DISPATCH loop -- an OOM allocating a Task, in a method that has just rented two large Color[] buffers -- propagates through the finally and never reaches it, discarding a slice failure already recorded. Both the discarded loser and a recorded failure the rethrow can no longer reach are logged from the finally now. **And logging the loser from the worker was a new deadlock.** Debug.unityLogger.logHandler is public and settable, and a handler that marshals to the main thread is an ordinary consumer pattern -- this package ships UnityMainThreadDispatcher for exactly that. The worker would block on a main thread sitting inside countdown.Wait(). Logging moved to the main thread, after the wait. **The branch gate re-derived the production condition instead of observing it.** Correct today, character for character, and silently wrong the moment ThreadedScale changes. The seam already reports every slice, so the test counts them: one slice is the single-threaded branch, whatever the expression upstream says. **A read seeds the newest epoch, so one absurd timestamp poisoned the tracker permanently.** GetPeak(1e30f) clamps to long.MaxValue, after which every ordinary Record is older than the ring and dropped, forever. A record that finds nothing live re-seeds instead -- there is nothing to protect, so the failure becomes transient. **The scene-unload fix had no coverage of the scene unload.** The fixture called the sweep directly; deleting the sceneUnloaded subscription left it green. A real CreateScene/UnloadSceneAsync round trip covers it, ignored in EditMode where there is no player loop. The hook also clears the cache at SubsystemRegistration, because a session with Domain Reload disabled otherwise carries the previous session's entries in. Smaller: OldestLiveEpoch avoids the wrap rather than detecting it, so it does not depend on unchecked arithmetic; the CHANGELOG entry said "alive for the process" where only an unloaded scene's objects are dropped; the pooling guide said 64 buckets where the ring is 66; the assertion whose false comment I deleted last commit is now the exact one the registry guarantees; and the untracked GameObject in the tag test would have turned one failure into a leak-guard line as well. #635: Tests/Editor/AssetProcessors leaves the baseline too -- 1,144 runs across 262 files down to 1,027 across 250. This tree is nearly all genuine rationale about test isolation, so it is a conversion sweep rather than a deletion one; the single run the fixer declined carries `/* ... */` in its own text and became the XML doc it should always have been. Refs #691, #693, #643, #635 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The third pass found the poisoning fix was asymmetric. Gating the re-seed on "nothing is live" only covered the case where a READ had established the epoch. `Record(1e12f, 1)` seeds the newest epoch AND leaves a live sample, so every ordinary reading after it is more than a window older, is refused, and the epoch it is compared against never moves again. SampleCount stays 1 forever. Same defect, one branch over. The gate is gone. A sample more than a whole window older than the newest is a clock that went backwards further than the ring can represent -- a reset, not a straggler, because a jittery clock's samples stay inside the window -- so it restarts the ring rather than being refused. That also still stops it aliasing a live bucket's slot, which is what the drop was protecting. The test that pinned the drop pins the restart, and still discriminates: on the original code the two records left SampleCount 2 with one bucket holding one of them. Four comments that were wrong or unqualified: - The PoolLifecycleHooks assertion holds because SetUp CLEARS the registry, not because Unregister removes exactly one -- it also prunes every dead weak reference in the same pass, and never breaks. My replacement comment credited a guarantee the registry does not make. - The finally in TextureScale called dropping a second failure "the defect this whole change is about" while itself dropping a third. It says what it keeps now, and the two log calls are guarded: they run during unwinding, so a consumer's throwing log handler would replace the exception the caller is about to receive. - The tag cache's XML remark and the helper docs claimed a scene unload drops the entry, without saying that the hook doing it is a runtime one and never runs in the editor outside play mode. - The RollingHighWaterMark class remarks still described the drop. And two hazards: clearing the tag cache at SubsystemRegistration is order-dependent against a consumer seeding it from its own hook at the same load type, so it sweeps the destroyed entries instead -- which is what the previous session left and nothing else; and the scene the new fixture creates is tracked now, or a failure before the unload would leave a loaded scene and a live object behind for every test after it, somewhere the leak guard cannot look. Refs #693, #691, #643 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Collaborator
Author
|
Recording the PlayMode evidence from The gate that came out in #692 failed every PlayMode leg on all four editors for
Twenty-one checks completed, zero failures. The three PlayMode legs that did finish are three of the four that used to fail, which is the measurement the issue asked for. Two things that measurement did not come from, and are worth saying so nobody re-derives them:
Three adversarial review passes over the branch found 23 defects in my own work, all fixed; the notable ones are in the commit messages rather than here. The one worth flagging to a reader: the first fix for the swallowed |
The fourth pass caught that the CyclicBuffer half of the #637 work was solving a problem this type does not have, and it is right. A CyclicBuffer allocates NOTHING from its stated capacity: the constructor takes an empty List and Add grows it one element at a time, bounded by the items the payload actually delivers. `new CyclicBuffer<Vector3>(int.MaxValue)` costs about 32 bytes. So the refusal refused nothing dangerous, and it cost two real things: a legitimately large, sparsely filled buffer -- a 2,000,000 capacity trail holding 500 points -- stopped round-tripping, and the root path disagreed with the nested one, where CyclicBuffer's own [ProtoAfterDeserialization] restores the same field with no check at all. The three sites go back to raising the capacity to hold what arrived, which is what the two binary paths always did and what the JSON path now does too. The test that pinned the refusal is replaced by one that pins the property the decision rests on -- a stated capacity allocates nothing -- so if that ever stops holding, the reason to add a limit fails loudly rather than being re-derived. Three smaller ones from the same pass: - EffectHandler destroyed a GameObject a self-cleaning component meant to keep. My destroyed-entry `continue` skipped the `shouldDestroyGameObject = false` beneath it, and the destroyed entry may BE the one that opted out -- reading CleansUpSelf is exactly what is no longer possible. A destroyed component means something already took it apart, so it counts as opting out. - The logging guard in TextureScale excluded OutOfMemoryException, copying a teardown-code idiom into a finally that is already unwinding a real failure. A log handler runs out of memory exactly when the caller most needs the exception it is about to receive. - Two decisions recorded where they can be read: a ring restart costs the whole live window, which is the loud failure chosen over the quiet one; and sweeping rather than clearing the tag cache keeps an entry naming something the session did not destroy, which ClearTagCache is the answer to. Its one finding I am not acting on is the claim that the restored spatial gate is known-red. That was true of the sibling branch, which does not carry the tracker fix. On this branch, run 33724... has 2022.3.45f1, 6000.3.16f1 and 6000.5.2f1 playmode all green -- three of the four legs that used to fail. Refs #637, #640, #691, #693 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The fifth pass found the one judgment call in the last commit was wrong, with a scenario I had not considered. Reading a destroyed entry as "it cleaned itself up" is unsound in the direction that leaks: two components that both opted IN to destruction, where the first one's teardown chain destroys the second, now leave the loop vetoing on the second's behalf and the GameObject survives -- one that was destroyed before any of this. The null guard is there to stop the read from throwing. It does that and nothing else now. Also dropping ACyclicBufferAllocatesNothingFromAStatedCapacity: it pins the premise the reverted limit rests on, and CyclicBufferTests.IntMaxCapacityOk has pinned exactly that since before this branch -- same constructor, same int.MaxValue, then fifty adds. The serialization fixture points at it rather than asserting it twice. Refs #640, #637 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
4 tasks
The gated IL2CPP standalone tier caught this, which is what that tier is for. AnImmutableBitSetCapacityBeyondItsWordsIsBoundedByThem built its payload by serializing a real ImmutableBitSet and patching the two bytes encoding its capacity. That works on Mono and does not survive a stripped IL2CPP player: 2021.3.45f1 threw ExecutionEngineException : Attempting to call method 'ProtoBuf.Internal.StructValueChecker`1[[...ImmutableBitSet...]]::.cctor' for which no ahead of time (AOT) code was generated and 6000.5.2f1 did not throw at all -- it returned a set carrying no words, so the assertion read Capacity 0 instead of 64. The property the test wants has nothing to do with the wire. The constructor is where the invariant is established, and its only non-test callers are BitSet.ToImmutable, which maintains it, and the deserialization surrogate, which restores the two members independently and so can disagree. Asserting it there is backend-independent and says what it means. The AOT gap the test found is real, pre-existing, and nothing else in the suite was exercising it -- every EditMode and PlayMode leg on all four editors passes the same round trip. It is filed as #696 with both symptoms rather than folded in here, because a stripped player silently returning an empty value is a save file that loads wrong, and that is a bigger question than this branch. Refs #637, #696 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Collaborator
Author
|
Green: 32 checks, zero failures, including all four gated IL2CPP standalone legs, both The eight editmode/playmode legs are the answer to #693. The gate removed in #692 failed every PlayMode leg on all four editors; all four pass now, and the whole matrix has been green on every push since. What the gated tier caught, which the main matrix did notThe standalone tier runs after
The test had built its payload by serializing a real value and patching the two bytes encoding its capacity, which is a dependency on the wire encoding and therefore on the backend. The property it wanted is constructor arithmetic; it asserts that now, and passes everywhere. The AOT gap is real, pre-existing, and nothing else in the suite was exercising it — every EditMode and PlayMode leg on all four editors passes the same round trip. Filed as #696 with both symptoms, because a stripped player returning an empty value where an older one throws is a save file that loads wrong, and that is a bigger question than this branch. ReviewFive adversarial passes over the branch, 30 defects found in my own work, all addressed. The two worth a reader's time, because both are cases where a fix was the defect:
|
Owner review on #695: "Why can't we index once and put back? Does repeatedly re-indexing really do anything? Can't we like, ref-index? Here and elsewhere." Yes, and on the one path this whole PR exists to make free. RecordCore read _buckets[index] eleven times -- six in one branch, five in the other -- and AdvanceTo three and five in its two loops, each an address computation and a bounds check, on every pooled rent. **Fix:** ref locals there, and at every other place a sweep of Runtime/ and Editor/ found the same struct array element read three or more times in a row with nothing between the accesses that can replace the array: - IListSortPowerPlus.BuildPowerSortPlusRuns, nine accesses across two loops. - Its heap validation, six accesses over two elements. The bounds guard had to come out of the && chain first -- a reference taken above the test that makes it legal is a reference out of range. - Cache's LFU victim scan, EvictEntry, AddToProbation, AddToProtected. - RestorableGlobal's BorrowCore, its constructor's free-list init, TakeSlot's post-resize init, and ReleaseCore's tail. Three of those four are taken deliberately AFTER an Array.Resize, and the fourth after the comparer. **Where it is deliberately NOT applied.** The reference aliases the array, so it lives only as long as that array does. Cache.SetUnlocked and Cache's disposal loop each run caller-supplied code between accesses to one element, so they keep the repeated indexing -- though the hazard there is weaker than it looks and the skill entry says so: _lock is a ReaderWriterLockSlim with LockRecursionPolicy.NoRecursion in BOTH compilation modes, so a re-entrant mutator throws before it can reach Grow. That makes those two conservative rather than required. RestorableGlobal.ReleaseCore is the live case -- lock (_gate) is a re-entrant Monitor and a re-entrant borrow does reach Array.Resize -- which is why its reference is taken below the comparer rather than above it. SevenZip is vendored verbatim; List<Rect> in SpriteSheetAlgorithms has no ref indexer, CollectionsMarshal.AsSpan being .NET 5+. SerializableType's three reads are a one-time static init, left alone as not worth the noise. **Rule:** none, and that is the decision rather than an omission. Nothing static can see whether a callee replaces the array, and a WUH### is capped at warning-and-suppressible so it is never wrong in a way that costs the reader -- here, a silently lost write. The guidance goes in avoid-allocations.md instead, beside the value-type section, hazard first. Refs #693 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Eli Pinkerton (wallstop)
deleted the
fix/spatial-allocations-and-worker-faults
branch
September 3, 2026 20:30
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.
Why: A pooled rental — the cheapest primitive the package has — allocated on a growth boundary for its pool's first ten thousand rents, and each pool then retained 131 KB of usage samples for the process.
What:
TextureScale.Bilinear/Pointreport a failed worker slice instead of returning a partly scaled texture with no log.Helpers.Find(tag)lets go of the object it cached when the scene unloads;Helpers.ClearTagCache()drops the rest.CyclicBufferandImmutableBitSetno longer trust a capacity a payload only claims.Fixes #693
Fixes #691
🤖 Generated with Claude Code
Note
Medium Risk
Touches hot pooling, parallel texture scaling, and multi-threaded relational field assignment caches; failures could affect memory, image correctness, or concurrent Awake/OnEnable assignment.
Overview
Pool purge tracking no longer stores up to ten thousand timestamped samples per pool (which caused allocations on growth and ~131 KB retained per pool).
RollingHighWaterMarknow uses a fixed ring of time buckets, so recording stays O(1) and allocation-free and purge sizing averages every sample in the rolling window, not a capped recent slice.Helpers.Find(tag)drops cache entries when the cached object is destroyed (including on scene unload viaSceneManager.sceneUnloaded), andHelpers.ClearTagCache()clears the whole cache.[SiblingComponent]/[ChildComponent]/[ParentComponent]/[ValidateAssignment]field metadata caches useConcurrentDictionarywhen notSINGLE_THREADED.TextureScale.Bilinear/Pointwait for workers, capture the first slice failure, log a second if needed, and rethrow instead of returning a partly scaled texture silently.CyclicBufferJSON/proto restore raises stated capacity to fit delivered items (aligned with binary paths).ImmutableBitSetclamps capacity to the word array actually provided.EffectHandlerskips null/destroyed cosmetic components when tearing down. Hot paths usereflocals for repeated struct-array indexing (Cache,RestorableGlobal, PowerSort+), with guidance added in.llm/skills/avoid-allocations.md. Editor asset-postprocessor tests are mostly comment/doc clarifications; no new product behavior there.Reviewed by Cursor Bugbot for commit 7372931. Bugbot is set up for automated code reviews on this repo. Configure here.