Hold a settings scope per flow instead of per thread - #857
Merged
Conversation
Rafael-SOWNet
marked this pull request as ready for review
August 9, 2026 20:08
This was referenced Aug 9, 2026
MathS.Settings kept its fourteen values in [ThreadStatic] fields, so a scope stopped
at the first await:
using var _ = MathS.Settings.MaxExpansionTermCount.Set(1);
// 1
await Task.Delay(20).ConfigureAwait(false);
// 2000 -- the default, silently
The continuation resumed on a pool thread that had never seen the scope. The same
mechanism ran the other way: a thread going back to the pool still carried whatever
scope was left on it, so the next caller to borrow it could compute under a precision
or codomain it never asked for. Neither surfaces as an error; both change the answer.
The obvious repair -- AsyncLocal<Setting<T>> on the static field -- is wrong. An
AsyncLocal flows the reference, and Setting<T> was mutable, so two concurrent tasks
would have pushed onto one shared stack. What is per-flow has to be the value stack
itself, so the stack moved inside Setting<T> as an immutable chain behind an
AsyncLocal, and the static fields became ordinary singletons. They also had to stop
being lazily created: `field ??= default` on a non-thread-static field races, and a
scope opened on the loser of that race would vanish.
Scopes are identified by a counter rather than by their frame. Releasing a scope that
is not on top rebuilds the frames above it, and a rebuilt frame is a different object,
so keying on the frame left everything above it impossible to release afterwards --
which the out-of-order test caught. Dropping the Guid this replaces is most of why
Set() got cheaper.
Measured, 20M reads and 2M scopes:
read PrecisionErrorZeroRange.Value 7.3-8.0 ns -> 1.2 ns
read DowncastingEnabled.Value 0.79 ns -> 0.96 ns
Set() + Dispose() 388-395 ns, 32 B -> 46 ns, 112 B
a Simplify workload 5.6-5.8 s -> 5.5-5.6 s, +1.5% allocated
Reads got faster because the old getter re-tested a [ThreadStatic] field for null on
every access. Opening a scope allocates more, since assigning an AsyncLocal copies the
flow's value map, but reads outnumber scope openings by orders of magnitude.
The recursion-depth counters and per-thread scratch caches elsewhere keep
[ThreadStatic] deliberately: an lHopital depth must not follow a call into a sibling.
RewriteRecording has the same per-thread ambient shape and is left alone here; its
documentation already says "on this thread".
Verified: 6069 C# tests and 130 F# tests pass, and the public surface is unchanged.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Rafael-SOWNet
force-pushed
the
fix/settings-asynclocal
branch
from
August 10, 2026 02:21
8791361 to
22c1bdf
Compare
Rafael-SOWNet
added a commit
that referenced
this pull request
Aug 10, 2026
Closes #859. RewriteRecording kept its ambient scope in a [ThreadStatic] field and documented the consequence rather than fixing it -- "A synchronous scope, and it has to be. Do not await inside one." It no longer has to be. The scope is an AsyncLocal, as MathS.Settings now is and as the cancellation token in MathS.Multithreading already was. The trap #857 flagged applies here and is why this is more than a field swap. An AsyncLocal flows the reference, so once the pointer reaches child tasks two of them can report to one recording at once, and the steps were accumulating into a List. That is a torn write, not a merged list. The store is a ConcurrentQueue, `closed` is volatile since it is now read from flows other than the one that set it, and Steps copies out rather than handing back a live view. One existing test encoded the old semantics and is rewritten rather than deleted: ARecordingOnOneThreadDoesNotSeeAnother started a thread inside an open recording and asserted its work was not collected. ExecutionContext flows to a manually started thread, so that work is now collected -- which is the point of the change, not a regression of it. It becomes WorkStartedUnderARecordingIsCollectedWhereverItRuns, and the isolation it was really reaching for is covered by SiblingRecordingsDoNotSeeEachOther. RewriteAllocationTest still passes untouched, so being off is still free: one ambient read per rule set, nothing allocated, which is what #746 asks of every layer above the tree. Verified: 6064 C# tests and 130 F# tests pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Rafael-SOWNet
added a commit
that referenced
this pull request
Aug 10, 2026
Closes #859. RewriteRecording kept its ambient scope in a [ThreadStatic] field and documented the consequence rather than fixing it -- "A synchronous scope, and it has to be. Do not await inside one." It no longer has to be. The scope is an AsyncLocal, as MathS.Settings now is and as the cancellation token in MathS.Multithreading already was. The trap #857 flagged applies here and is why this is more than a field swap. An AsyncLocal flows the reference, so once the pointer reaches child tasks two of them can report to one recording at once, and the steps were accumulating into a List. That is a torn write, not a merged list. The store is a ConcurrentQueue, `closed` is volatile since it is now read from flows other than the one that set it, and Steps copies out rather than handing back a live view. One existing test encoded the old semantics and is rewritten rather than deleted: ARecordingOnOneThreadDoesNotSeeAnother started a thread inside an open recording and asserted its work was not collected. ExecutionContext flows to a manually started thread, so that work is now collected -- which is the point of the change, not a regression of it. It becomes WorkStartedUnderARecordingIsCollectedWhereverItRuns, and the isolation it was really reaching for is covered by SiblingRecordingsDoNotSeeEachOther. RewriteAllocationTest still passes untouched, so being off is still free: one ambient read per rule set, nothing allocated, which is what #746 asks of every layer above the tree. Verified: 6064 C# tests and 130 F# tests pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.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.
MathS.Settingskept its fourteen values in[ThreadStatic]fields, so a scope stopped at the firstawait:The continuation resumes on a pool thread that has never seen the scope. The same mechanism runs the other way, too: a thread going back to the pool still carries whatever scope was left on it, so the next caller to borrow that thread can compute under a precision or codomain it never asked for. Neither surfaces as an error; both change the answer.
The values now live in an
AsyncLocal— which is what the cancellation token inMathS.Multithreadingalready used, so this brings settings in line with a decision the codebase had already made.Why not just
AsyncLocal<Setting<T>>on the fieldBecause that is wrong. An
AsyncLocalflows the reference, andSetting<T>was mutable, so two concurrent tasks would have pushed onto one shared stack and corrupted each other — losing the one property the per-thread field did give.What has to be per-flow is the value stack itself. So the stack moved inside
Setting<T>as an immutable chain behind anAsyncLocal, and the static fields became ordinary singletons. They also had to stop being lazily created:field ??= defaulton a non-thread-static field races, and a scope opened on the loser of that race would silently vanish.Scopes are identified by a counter rather than by their frame object. Releasing a scope that is not on top rebuilds the frames above it, and a rebuilt frame is a different object — so keying on the frame left every scope above it impossible to release afterwards. The out-of-order test caught exactly that. Dropping the
Guidit replaces is most of whySet()got cheaper.Behaviour
awaitTask.Runstarted under itThe second row is the one that can surprise. Work started inside a scope now runs under it — what the code says, and almost always what was meant, but a caller who parallelised inside a scope and relied on the child not seeing it will notice. Every row above is a test in
SettingsAcrossAsync.Cost
Measured over 20 000 000 reads and 2 000 000 scopes, two runs each:
PrecisionErrorZeroRange.ValueDowncastingEnabled.ValueSet()+Dispose()SimplifyworkloadReads got faster, not slower: the old getter re-tested a
[ThreadStatic]field for null on every access, and that costs more than the async-local lookup replacing it. Opening a scope allocates ~3.5× more, because assigning anAsyncLocalcopies the flow's value map — but reads outnumber scope openings by orders of magnitude, and the end-to-end workload is within noise.Deliberately not changed
The recursion-depth counters and per-thread scratch caches elsewhere (
lHopitalDepth,Gruntz.depth,FastExpression.scratch, the constant caches) keep[ThreadStatic]. A recursion depth must not follow a call into a sibling task.RewriteRecordinghas the same per-thread ambient shape and is left alone here — its documentation already says "on this thread" — but it is worth a look separately.Verification
PublicApiSurfaceTestgreen,PublicApi.txtuntouched)Breaking, so it wants the 2.0 window; recorded in
BREAKING-CHANGES.mdwith the migration and the measured before/after.🤖 Generated with Claude Code