Repository navigation
Preserve configured scheduler for queued TaskFlow callbacks - #60
dombrovsky wants to merge 1 commit into
Conversation
Recommendation: hold performance clearance for this head before 1.1.0 stable; investigate or explicitly accept the queued-throughput cost before merging. Production code and benchmark infrastructure are unchanged. Compared The complete repeat, with affinity fixed to CPUs 0–1 for both revisions, flags three material slowdowns under the repository's advisory heuristic:
Variability: the first pair (affinity 0–2) classified all timing differences as inconclusive. The fixed-affinity signals above survive restoring excluded samples: all-eight-iteration time increases are 50.7%, 58.5%, 33.0% respectively. Retained samples range from 6–8. The reporter's CV/three-standard-error checks and 20% materiality threshold are advisory, not formal significance tests. One launch, baseline-first drift, shared-host load/power, and denied high priority limit attribution and precision. Other focus areas: synchronous sequential means rise 10.6–12.1%, but are inconclusive. Async/no-token shows a smaller lower-variance +152.50 ns (+8.1%), throughput −7.5%. Enqueue-only improves in mean by 161.17 ns (−46.3%), submissions/s +86.1%, but candidate CV is 26%: not a reliable speedup, and allocations are unsupported. Other producer/middleware timings are inconclusive. All 16 fixed-affinity results: absolute and percentage time, throughput, and allocation changes
Reproduction: Burst timing CVs: 3.2% / 12.7%; four producers: 2.1% / 10.5%; async/live: 11.7% / 1.2%. The four-producer baseline excludes two slow samples, making its retained-sample +108.6% estimate larger than its raw +58.5%. Raw sensitivity checks include all eight workload iterations without BDN's tiny overhead subtraction. The first, noisy pair also showed adverse changes in those cases: +39.4%, +137.6%, +22.5% respectively. MemoryDiagnoser allocation deltas have no reported uncertainty; treat them as observed managed bytes/op, not statistical findings. No enqueue-only allocation claim is valid. Provenance: harness SHA-256 |
|
Investigation update — supersedes my earlier performance assessment. The reported +53% burst / +109% four-producer increases are not established code regressions. The stronger measurements did not reproduce those magnitudes. I have not isolated a confirmed cause for the remaining possible async cost or validated a production fix, so PR #60 remains unchanged at What I established:
The +17% async/no-token observation remains a possible smaller regression; I am not claiming it disappeared. Three launches, baseline-first drift, and uncontrolled host scheduling/power are insufficient to attribute it confidently or certify release performance. The burst launch-mean ranges overlap (parent 384–443 vs PR 395–447 ns/op), as do four-producer ranges (610–677 vs 622–682 ns/op). Gated enqueue-only measurements had many excluded outliers; its apparent improvement is not a dependable speedup. I tested three isolated changes: restoring the original default-scheduler wait; allowing local pool queues for pending callbacks; and inlining only internal completion bookkeeping while preserving the suppressed-ExecutionContext boundary. They passed the 31-case scheduler regression fixture on both .NET 8 and 10, but none consistently improved the three-launch results. Internal-marker inlining also worsened the four-producer mean by 33.4% in its confirmation. All three remain unpublished experimental branches; no production or benchmark-infrastructure change is being proposed from this investigation. Recommendation: I withdraw the earlier categorical performance-hold rationale based on the large single-launch percentages. This is not unconditional merge/performance clearance: the smaller async observation and the already documented full-suite baseline failures remain open. If performance is a stable-release gate, obtain controlled-runner, multi-launch paired evidence before choosing a code change. The repository's existing harness already supports that; no new benchmark infrastructure is needed. Absolute and percentage throughput/allocation results for the stronger parent/head confirmationThroughput is the reciprocal of the amortized mean, not an independent measurement. Managed allocation figures have no reported uncertainty; enqueue-only allocation attribution is unsupported. Three warmups and twenty actual iterations per launch; three launches per side. Outlier filtering retained 50–60 of 60 measurements; those are not sixty independent process launches.
Experimental changes: three-launch time changes relative to the unchanged PR headEach experiment used the same four-case confirmation and matched settings within its pair. Do not pool different comparison runs. These figures explain why I did not ship a speculative tuning patch.
All full-scope results, A/A results, raw BDN JSON, per-launch measurements and provenance are retained in |
|
Profiling follow-up for This is focused profiling of four sensitive cases (sequential async/no token, burst, four producers, and enqueue-only). The earlier benchmark comparison covered all 16 compatible cases. Both revisions used the identical existing common harness ( What the profiles show
Enqueue-only defers startup work until the gate opens; its allocation metric remains unsupported. Four-producer contention decreased in this run, so increased contention does not explain a slowdown here.
Variability and limits Collected 8 EventPipe stack traces, 4 short TPL task traces, and 2 GC/allocation traces; all report zero lost events, and stack traces report zero missing sampled stacks. Analysis excludes startup/pilot/warmup and uses actual-workload markers. EventPipe samples include parked threads: these are not CPU-utilization percentages, and kernel CPU/context-switch traces were not collected. Enqueue-only has only 32/45 sampled stacks, insufficient for ranking small hotspots. Instrumented elapsed times are not acceptance benchmarks. Thread-pool hill climbing changes worker targets during measurement even with processor count fixed at 2 (sequential traces: parent 2–5, head 2–8). This demonstrates differing pool state, but does not prove how much of the timing variance it causes. The earlier parent-vs-parent burst comparison also moved −31.2% with no code change. A smaller sequential latency cost remains possible; neither the original +53%/+109% claims nor a precise replacement timing penalty is established. Fix/merge recommendation: keep the explicit custom-scheduler dispatch required by #48. A Default-scheduler-only return to the original wait path can avoid the new allocation/dispatch pattern; the earlier prototype reduced burst allocation 582 → 536 B/op, but did not establish a timing improvement (its burst timing was worse in that noisy comparison). Do not merge that speculative optimization, inline user callbacks, or replace this with |
Queued TaskFlow callbacks now enter through the configured
TaskSchedulerafter an incomplete predecessor. This is a correctness fix for 1.1.0, referencing #48; the issue remains open.The old
RunAfterPreviousawaited the predecessor withConfigureAwait(false), then called the user delegate from that continuation. FIFO survived, but scheduler identity did not. Pending operations now use an unconditionalContinueWithonOptions.TaskScheduler, followed by the existingUnwrapand completion tail. Completed predecessors retainStartNewand its synchronous rejection behavior. Predecessor faults are observed without propagating them to later operations. There is no extra scheduling task per operation, public API change, Polly change, or change to the finalawait taskFunc(...).ConfigureAwait(false).ConfigureAwait(true)was tested and is insufficient: a scheduler-installed non-defaultSynchronizationContextwins over scheduler capture. The regression context posts raw callbacks onto the same physical worker thread, whereTaskScheduler.Currentis the default scheduler. Explicit task scheduling preserves callback-entry scheduler identity while leaving user awaits alone: capturing user code follows normal context/scheduler precedence;ConfigureAwait(false)can escape both. Scheduler identity, physical thread, synchronization context, and execution context remain distinct.The new fixture contains 25 deterministic cases using a non-inlining single-thread scheduler, gates, and queue markers. Coverage includes completed/pending successful, faulted, and canceled operations; FIFO and logical non-overlap; callback placement and capturing/escaping awaits; context precedence; operation-specific and suppressed ExecutionContext; caller cancellation, asynchronous shutdown and zero-timeout synchronous disposal; scheduler rejection/disposal and subsequent recovery; non-reentrant entry; and the adapter's independent scheduling contract. Existing dedicated/current-thread, execution-context, middleware, and disposal conformance tests also ran in the full suite.
Failure evidence before the fix
With production code restored to the original implementation, the final
PendingPredecessor_CallbackEntersConfiguredSchedulerregression failed both cases on each ofnet8.0andnet10.0:A separate production-code experiment changing only the predecessor await to
ConfigureAwait(true)on .NET 10 passed the case without a synchronization context and failed the case with one. The final fix passes all 25 cases on both targets. No sleeps or timing-based scheduling assumptions are used in the new fixture.Validation
dotnet restore TaskFlow.sln: passed.dotnet build TaskFlow.sln -c Release --no-restore: passed, zero warnings/errors; includesnetstandard2.0,net8.0, andnet10.0library builds.dotnet test TaskFlow.Tests/TaskFlow.Tests.csproj -c Release --no-build --filter FullyQualifiedName~TaskFlowSchedulerPlacementFixture: 25/25 passed per target, 50 total.dotnet format whitespace TaskFlow.sln --no-restore, its--verify-no-changescheck, anddotnet format style TaskFlow.sln --diagnostics IDE0161 --verify-no-changes --no-restore: passed. Format reports the existing shared.shprojworkspace-load warning.git -c core.whitespace=cr-at-eol diff origin/main --check: passed. C# edits use the repository's CRLF convention; existing Markdown line endings are preserved.Full Release solution runs completed on both test targets. The existing full suite is not green. Core-project results:
main(15e3d31)-m:1 -p:TestTfmsInParallel=false)Every failing test name across the patched runs was also observed failing on unmodified main (full-suite and focused baseline runs). The failures occur in
CancellationScopeTaskSchedulerExtensionsFixtureandCancelPreviousTaskSchedulerExtensionsFixture, with existing 100 ms polling assertions observingWaitingForActivation; counts vary by run. The .NET 10 standard full run has the exact same eight failing test names on main and this branch. This is evidence of existing failures, not a claim that the full suite passed.All other core tests passed, including the dedicated/current-thread and adapter fixtures and the new regressions. Dependency-injection (30), logging (5), and time (70) tests all passed on each target in all full runs. There were no skipped tests. SDK: 10.0.403; test runtimes: .NET 8.0.0 and 10.0.14.
Full-run commands were
dotnet test TaskFlow.sln -c Release --no-buildon this branch,dotnet test TaskFlow.sln -c Release --no-restorein an isolated main worktree, and the serialized command above on this branch. TRX logging was enabled for each run.Compatibility and remaining risks
Pending operations are dispatched after predecessor completion. A scheduler rejecting that deferred dispatch now faults the operation's returned task with the original failure inside
TaskSchedulerException; it cannot silently run the callback on the thread pool. This also means rejection for a pending predecessor is delivered asynchronously, whereas rejection of the completed-predecessorStartNewremains synchronous. Keep the scheduler available through flow disposal. Cancellation remains cooperative, and each accepted delegate still receives its linked token when invoked. A user's explicit context escape is respected.The remaining validation limitation is the existing cancellation-test failures described above. This PR stays draft while the full-suite baseline is not green; it does not change unrelated middleware or timing-based tests.