[5.0] Drop [ThreadStatic] walker cache and allocate walkers on demand - #1814
Merged
Conversation
The hand-rolled GetInstance/Free pattern was repeated on 23 types, and its reuse safety rested entirely on Free manually nulling every field, guarded only by Debug.Assert. Four call sites had no try/finally at all, and none of the collection-owning walkers capped the buffer they retained per thread. Pooling is kept only where the pooled object owns a growable buffer. Those eight types now implement IResettable and rent through ObjectPool<T>, whose PooledObject<T> makes the release unconditional via using, and whose CanBeCached check drops an instance whose collection outgrew ObjectPool.MaxCachedBufferSize instead of pinning it for the life of the thread. The remaining 15 walkers take their state through constructors, so they can no longer arrive dirty. This also fixes RCS1187, which was reporting inconsistently: a fresh UseConstantInsteadOfFieldWalker started with CanBeConvertedToConstant set to false, suppressing the diagnostic, while a recycled one started with the value Free had reset it to and reported normally. Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
CanBeCached was checking List.Count after a balanced visit (always ~0), so oversized walkers were never discarded; use Capacity instead. Make PooledObject.Dispose idempotent, and align UnusedMemberWalker with Initialize. Co-authored-by: Cursor <cursoragent@cursor.com>
…eset PooledObject is now a ref struct so it cannot be stored in a field. Reset returns whether to cache, Free is a no-op on null or an already-cached instance, and RemoveAsyncAwaitAnalysis takes the walker out of the rental instead of embedding PooledObject. Co-authored-by: Cursor <cursoragent@cursor.com>
The ThreadStatic cache and the ObjectPool that replaced it were more protocol than the allocations were worth next to semantic-model work. Walkers take their state in constructors; StringBuilderCache is unchanged. Co-authored-by: Cursor <cursoragent@cursor.com>
dotnet format --severity info failed RCS1250 on target-typed new(). Co-authored-by: Cursor <cursoragent@cursor.com>
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.
Summary
The hand-rolled
[ThreadStatic] _cachedInstance+GetInstance()/Free()pattern was repeated on 23 types. Reuse safety rested entirely onFreemanually nulling every field, guarded only byDebug.Assert(a no-op in release); four call sites had notry/finally; and none of the collection-owning walkers capped the buffer they retained per thread.This PR deletes that cache. Walkers are constructed when needed. Reuse setters became constructor parameters or read-only properties, which removes the stale-state class of bugs.
RefactoringFlagsCacheis deleted. The defensivewalker.ContainsContinueStatement = false;inUseForStatementInsteadOfWhileStatementAnalyzeris gone, since the field can no longer arrive dirty.Both
StringBuilderCachecopies are left alone. They already have a size guard, and one is vendored Orang code.A shared
ObjectPool<T>was prototyped for the eight buffer-owning walkers, then dropped: those allocations sit next to semantic-model work, and the pool protocol (reset ordering, copy-safe dispose, async/ref struct) was more machinery than the savings justified. If a regression shows up,ContainsLocalOrParameterReferenceWalkeris the first place to look — it has 10 call sites, four of them inside loops inInlineLocalVariableAnalyzer.Two bugs the pooling was masking
UseConstantInsteadOfFieldWalker.CanBeConvertedToConstanthad no initializer, so a fresh walker started atfalse, makingShouldVisitfalse and suppressing the diagnostic, while a recycled walker started at thetruethatFreehad reset it to and reported normally. Whether RCS1187 fired therefore depended on whether the thread had run the analyzer before. The property now defaults totrue, matching whatFreealways restored. The visible effect is that astatic readonlyfield is now consistently reported when the containing type has a static constructor that does not assign it — the same answer the analyzer already gave when there was no static constructor at all. Covered by a new test and a ChangeLog entry.ConvertWhileToForRefactoringdouble-freed a walker. It calledFree(walker)and then returned from inside thetry, so thefinallyfreed the same instance again, handing one walker to two future callers. Dropping pooling there removes it.Test plan
dotnet build src/Roslynator.sln --no-incremental), 0 warnings, 0 errors[ThreadStatic] _cachedInstanceremains outside the twoStringBuilderCachefilesMade with Cursor