Add perf experiment pipeline and fix v2 connection pool regressions

[mdaigle]

Description

Two related changes: a new perf pipeline for A/B testing AppContext switches, and the first fix for regressions it surfaced in the v2 connection pool.

Perf experiment pipeline

The existing perf pipelines cover two use cases: PR vs main, and main vs a published baseline. In both, any AppContext switches apply to baseline and current alike, so they cannot measure the effect of the switch itself.

This adds a third use case: run the same commit against itself with a perf-sensitive switch on in one variant and off in the other.

• New eng/pipelines/perf/sqlclient-perf-experiment.yml with a switchUnderTest dropdown (default UseConnectionPoolV2).
• run-perf-tests.sh / .ps1 take a general --switch-under-test / -SwitchUnderTest.
• interleave_perf.py applies a per-variant runner-config override.

Two things worth calling out:

• This pipeline never ingests into Kusto. Its two variants are the same commit, so the results are not comparable to the trend data the other pipelines produce and would pollute it.
• Two processes are required. InProcessEmitToolchain in BenchmarkConfig.cs pins benchmarks to the host process, so AppContext switches cannot be varied within a single BenchmarkDotNet run.

The pipeline drops the useManagedSni / useConnectionPoolV2 / useOptimizedAsyncBehaviour parameters the other pipelines expose. ADO boolean parameters always emit a value, so leaving them in would fire the "flag ignored" warning on every run. The runnerconfig.jsonc defaults already match what those parameters defaulted to.

v2 connection pool fast path

Running the above with switchUnderTest: UseConnectionPoolV2 showed large regressions on open/close-heavy benchmarks: OpenAsyncConnection +273%, RapidOpenCloseSingleThreadAsync +98%, RapidFireOpenClose +46-85% with allocation deltas of +110-212%.

Three causes, all on the path taken when the pool already holds a usable connection:

• Every async open dispatched to the thread pool via Task.Run with no inline attempt first. v1 tries a non-blocking acquisition on the caller's thread (allowCreate: false) and only queues a pending request on a miss. The 3.6 us to 13.3 us jump on OpenAsyncConnection is thread pool round-trip latency for trivial work.
• GetInternalConnection creates a timer-backed CancellationTokenSource before it knows whether it will ever wait. That is an allocation plus a TimerQueueTimer registration, which also contends a shared lock at higher parallelism. This is the allocation delta on the uncontended benchmarks above; the pool-stress runners are a separate cause, covered below.
• GetInternalConnection is an async method, so it allocates a Task<DbConnectionInternal> even when it completes synchronously.

TryGetPooledConnectionInline performs the transacted-store and idle-channel lookups that GetInternalConnection begins with and returns null the moment neither can satisfy the request. Both the sync and async entry points try it first, so the common case avoids all three costs at once. It deliberately never calls OpenNewInternalConnection, so no caller thread blocks on network I/O.

Remaining differences after the fix

Latest run: 173 benchmarks, 10% threshold, 3 interleaved confirmation runs. 10 confirmed regressions, 3 unconfirmed, 51 improvements. 38 benchmarks flag on time or allocation. Allocation is reported only and never gates the build (compare_perf.py sets status from meanDeltaPct alone), so of the 31 allocation increases above 10%, 24 are on benchmarks that got faster.

Scoping first. Benchmarks with no pool checkout in the measured body (37) have a max allocation delta of +0.05%, and those amortising one checkout over real work (67) max at +1.41%. Zero above 10% in either group; all 31 are checkout-dominated. ParallelAsyncConnectionRunner parameterises pooling directly, so it isolates the pool from everything else in the same benchmark:

concurrency	alloc Δ, Pooling=False	alloc Δ, Pooling=True
10	+0.04%	+113.16%
50	+0.11%	+120.56%
100	+0.01%	+70.55%

The 38 flagged benchmarks break down as 30 concurrent pool growth, 3 threadpool saturation, 1 saturated async waiter allocation, and 4 noise. Each is covered below.

A. Concurrent pool growth (30 of 38) — benchmark defect, now fixed. The flagged ConnectionPoolStressRunner cases plus ParallelAsyncConnectionRunner.OpenConnectionsConcurrently. Both runners ClearAllPools() in [IterationCleanup], so every iteration re-established physical connections inside the measured body. v1 serialises pool growth behind a Semaphore(1, 1); v2 deliberately does not, so it creates more physical connections, finishes sooner, and allocates more. A physical connection costs the same in both pools (v1 57,275 B, v2 57,701 B), and dividing each benchmark's allocation delta by its Login7Count delta lands on 55.5-59 KB in every shape tested.

Rather than explain that away, ConnectionPoolStressRunner has been fixed (ab21302): it now pre-warms the pool to full capacity in [GlobalSetup] and no longer clears it between iterations, so its bodies only exercise checkout and return. Establishing a connection costs milliseconds against microseconds for a pooled checkout, so any creation left in the measured body swamps the cost the class exists to measure. The 27 ConnectionPoolStressRunner rows quoted here therefore predate that fix and will be re-measured on the next pipeline run. ParallelAsyncConnectionRunner is deliberately left as-is: it reproduces #601/#979, where concurrent open storms against a cold pool are the actual subject, and it has Pooling=false variants.

Three smaller benchmark defects were fixed alongside it, all of which also invalidate the stress rows quoted above:

• RapidFireOpenCloseSync added and RapidFireOpenClose renamed to RapidFireOpenCloseAsync (2ac2b2e, 2959100). The concurrent runner measured this workload on the async path only, so a sync/async split there could not be told apart from a scheduling difference. Result rows quoted elsewhere in this description predate the rename and appear under the old name. The old name still exists on main, so a PR-vs-main run will report this row without a delta rather than comparing it — which is correct, since the benchmark now pre-warms the pool where main clears it, and a delta across that change would be spurious.
• ConnectionPoolChurnRunner parameterised on pool depth (2104d10). It was pinned at one pooled connection, which is the one depth at which the two pools cannot differ in reuse order: v1 pops idle connections from a ConcurrentStack (LIFO) and reuses the connection just released, while v2 reads from an unbounded Channel (FIFO) and cycles through all of them. The runner now covers depth 1 and 100.
• PoolExhaustionRecovery now sizes itself as MaxPoolSize + Parallelism (9ede8e0). It was Math.Max(Parallelism, MaxPoolSize * 2), and since that second term is 100 or 200 while Parallelism never exceeds 25, the parameter had no effect: six configurations were only two distinct workloads. Parallelism is now the oversubscription amount, which is the depth of the queue of waiting callers.

ConnectionPoolRampRunner was added to test that claim rather than argue it. It keeps the cold pool but makes every caller hold its connection until all have connected, so both pools must create the same number of connections and the only remaining variable is how fast they get there:

	time Δ	alloc Δ
ColdStartRamp P=10 / 25 / 50	−75.23% / −80.62% / −82.20%	+0.08% / +0.58% / +0.64%
ColdStartRampAsync P=10 / 25 / 50	−73.42% / −80.93% / −82.89%	−2.62% / −0.17% / +0.80%

Holding connection count constant makes the allocation delta vanish and leaves v2 4-6x faster. That was the same behaviour RapidFireOpenClose scored as a 33-75% regression, because it held connections for zero time and so measured a burst that never needed the extra connections.

Two independent checks already agreed that the checkout path itself got faster, before any benchmark change. ConnectionPoolChurnRunner runs RapidFireOpenClose's exact inner loop — new SqlConnection / OpenAsync / dispose — single-threaded against a warm pool that is never cleared, and v2 wins there: −31.27% async and −31.54% sync, with allocation at +3.51% / +4.92% rather than +47–139%. (Those were measured at pool depth 1, before the depth axis above was added.) SqlConnectionRunner's pooled opens agree at −29% to −47%.

One caveat on reading the old stress numbers precisely: every Parallelism value (10, 20, 25) is below every MaxPoolSize value (50, 100), and the Math.Max(20, ...) floor leaves Parallelism 20 and 25 running identical workloads at both MaxPoolSize values. Those duplicate pairs report deltas 22.5 pp and 19.0 pp apart (+75.18% vs +52.68%, and +59.16% vs +40.12%), which puts this benchmark's noise floor at twice the 10% threshold.

B. Threadpool saturation on the sync wait path (3 of 38). SteadyStateOpenQueryClose P=50/Max=10 at +136.27%. A sync waiter blocks in mres.Wait(), and with AllowSynchronousContinuations off the returning thread must queue the wake into a pool whose workers are all blocked, so it waits on thread injection. Two controls isolate that:

variant, P=50 / Max=10	time Δ	alloc Δ
SteadyStateOpenQueryClose (threadpool, sync)	+136.27%	+5.87%
SteadyStateOpenQueryCloseDedicatedThreads (sync)	+5.61%	+8.00%
SteadyStateOpenQueryCloseAsync (threadpool)	−4.20%	+10.27%

Allocation is comparable across all three while time differs by 140 points, so the regression is not the pool's connection handling. ConnectionPoolThreadPoolPressureRunner varies only the thread floor and shows the same thing: MinWorkerThreads=8 gives +57.48% (3/3), MinWorkerThreads=128 gives +11.23% (2/3). The ample-pool variants stay under the threshold (Max=50 is −1.80%, Max=100 is +7.91%), so this is specific to Parallelism ≫ MaxPoolSize.

Accepted as an application-configuration boundary: an application should keep parallelism below the threadpool worker count, and pre-warming the threadpool is not the driver's job. Enabling AllowSynchronousContinuations was measured (−7.8%) and rejected, because OpenNewInternalConnection sits in the same retry loop and an inline resume could run a TCP connect, TLS handshake and login on a caller's Close() thread.

C. Saturated async waiter allocation (1 of 38). SteadyStateOpenQueryCloseAsync P=50/Max=10, +10.27% allocation while 4% faster. The only genuine per-operation allocation regression, and the only Async row with a meaningful delta (Max=50 is +0.19%, Max=100 is +0.77%). A saturation sweep holds flat at +24 B/op down to maxPool=25, then jumps to +1,241 (maxPool=10) and +1,481 (maxPool=5) on the async path only; an isolated microbenchmark predicted +10.05% for this shape against the +10.27% measured. 392 B of it is passing a cancellable token to ReadAsync: 104 B defeats UnboundedChannel's cached reader, 192 B is the CTS, 96 B its timer. Sync escapes because _syncOverAsyncSemaphore caps concurrent readers.

Not fixed here. Deferring CTS creation to the first blocking wait was implemented and measured: it saves 24 B of 1,481, because a saturated caller always blocks and so always needs the token. Removing the 392 B means reading with CancellationToken.None, which needs an explicit waiter queue so a timed-out waiter can deregister without stranding the connection a pending read would swallow. That is the same restructuring B needs, so it is one workstream rather than two.

D. Noise (4 of 38). The async large-data read benchmarks contribute the +68.23% (2/3) and two 1/3 flags, and SequentialXmlReadRunner.ReadXml adds a fourth at +11.55% (1/3). Across all 16 large-data rows the deltas scatter from −13.92% to +68.23% with allocation pinned at ≈0.00% throughout, and three of those rows are flagged improvements of comparable size; the same 5 MB payload reads +68.23% with a 1 MB buffer and −13.92% with an 8 KB buffer. Both runners take one checkout and then read, so the switch has nothing to act on after connect. The previous run's two 1/3 flags (MarsOverheadRunner, BeginTransactionRunner) did not reproduce. (These ran as AsyncLargeDataReadRunner; that runner has since been reorganised on main into LargeDataReadRunner/{Plaintext,AlwaysEncrypted}.)

Two caveats worth stating. SteadyStateOpenQueryClose P=50/Max=10 moved from +66.61% to +136.27% between runs (both 3/3): this is a tail-latency effect, not a shifted median, so the magnitude is not a stable number to quote. And the MinWorkerThreads=128 control lands at +11.23% rather than parity, because its 50 blocked waiters are still threadpool threads competing for scheduling, which the dedicated-thread variant avoids.

Separately, I investigated and ruled out redundant liveness probing as a cause of the original regressions. v2 calls IsLiveConnection up to 3x per cycle vs v1's 1x, but IsConnectionAlive is gated by a 5 ms window and a successful check resets the timer, so the extra calls collapse to a few DateTime.UtcNow reads.

Issues

N/A

Testing

Two unit tests added to ChannelDbConnectionPoolTest:

• GetConnectionAsync_WithIdleConnection_ShouldCompleteInline is the meaningful one. It asserts the TaskCompletionSource is already completed when TryGetConnection returns, which is impossible to observe if the work was dispatched via Task.Run. Verified it fails without the fix.
• GetConnection_WithIdleConnection_ShouldReturnInline covers the sync path.

The full connection pool unit test suite passes: 359 tests, green on 3 consecutive runs after merging main.

Two fixes came out of that merge and the review pass:

• Soft connects were not counted on the new fast path. TryGetPooledConnectionInline activated the connection without recording the request, so a fast-path checkout went uncounted while its matching return still emitted a soft disconnect. Main's new DbConnectionPoolInstrumentationTest caught it (152 counted instead of 400). The count now happens immediately before activation, matching GetInternalConnection.
• ConnectionPoolRampRunner could hang. Both ramp benchmarks only reached their rendezvous on the success path, so a caller that threw before signalling left the rest waiting forever, which would stall the perf job. Signalling now happens in a finally and both waits are bounded.

Not automated: the perf improvement itself. Plan is to re-run the experiment pipeline on this branch with switchUnderTest: UseConnectionPoolV2 and confirm the async cases move back toward parity.

One gap worth flagging: the full unit test suite hangs in SimulatedServerTests on macOS. I confirmed this is pre-existing by reproducing the identical hang on a clean tree, and those tests do not use the v2 pool.

Guidelines

Please review the contribution guidelines before submitting a pull request:

• Contributing
• Code of Conduct
• Best Practices
• Coding Style
• Review Process

[mdaigle]

https://dev.azure.com/SqlClientDrivers/ADO.Net/_build/results?buildId=167208&view=results

[Copilot]

Pull request overview

Adds a new perf “switch experiment” pipeline to A/B test a single runner-config/AppContext switch on the same commit, and applies an optimization to the v2 channel-based connection pool to remove avoidable allocations and thread-pool dispatch on the idle-connection fast path.

Changes:

• Introduces sqlclient-perf-experiment.yml to run baseline/current as the same source with exactly one switch flipped (no Kusto ingestion by design).
• Updates perf runner scripts (run-perf-tests.sh / .ps1) and the interleaving orchestrator to support per-variant RUNNER_CONFIG overrides for switch A/B.
• Adds TryGetPooledConnectionInline to ChannelDbConnectionPool.TryGetConnection to satisfy idle/transacted requests inline and avoid Task.Run/CTS/Task<T> allocations; adds unit tests covering sync and async inline completion.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTest.cs	Adds unit tests verifying idle-connection requests complete inline for both sync and async paths.
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs	Adds a pooled-connection inline fast path (TryGetPooledConnectionInline) to avoid async state machine + CTS + thread-pool dispatch when an idle/transacted connection is immediately available.
eng/pipelines/perf/sqlclient-perf-experiment.yml	New manual perf pipeline that runs the same commit twice with one switch forced off vs on.
eng/pipelines/perf/scripts/run-perf-tests.sh	Adds --switch-under-test mode, generates per-variant runner configs, and wires them into interleaved/sequential runs.
eng/pipelines/perf/scripts/run-perf-tests.ps1	Windows equivalent of switch-under-test A/B mode, including per-variant runner config generation and plumbing.
eng/pipelines/perf/scripts/interleave_perf.py	Adds optional per-variant environment overrides so baseline/current subprocesses can use different RUNNER_CONFIG values.
eng/pipelines/perf/README.md	Documents the experiment pipeline, its constraints, and why it must not be ingested into Kusto.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.

[Copilot]

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.

Suppressed comments (1)

src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTest.cs:1561

• GetConnection_ConcurrentHeldLoad_GrowsPoolToDemand asserts factory.Created == parallelism, but RunWorkersAsync does not synchronize worker start. Because tasks can begin at different times, some workers may start after others have already returned a connection and end up reusing it, causing factory.Created to be < parallelism and making this test timing/scheduling-sensitive (potentially flaky in CI). Consider adding a start barrier (e.g., CountdownEvent + ManualResetEventSlim) so all workers contend concurrently before any can return, or relax the assertion to a range that still detects the “serialized to 1 connection” failure mode without requiring perfect simultaneity.

            // Act: workers hold their connections, so none can be reused and the pool must grow.
            await RunWorkersAsync(pool, parallelism, iterationsPerWorker, holdMilliseconds: 25);

            // Assert: one connection per concurrent caller, and no more.
            Assert.Equal(parallelism, factory.Created);

[Copilot]

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.

Suppressed comments (1)

eng/pipelines/perf/scripts/run-perf-tests.ps1:150

• The comment above the baseline-selector validation still says there are "two baseline selectors" and that "the two baseline selectors are mutually exclusive", but this script now supports three selectors (BaselineVersion, BaselineSourceRef, and SwitchUnderTest). This makes the comment misleading for future maintenance and review.

# The two baseline selectors describe different builds of the same "baseline" pass, so requesting
# both is always a mistake; fail fast rather than silently honouring one of them.
if ((-not [string]::IsNullOrEmpty($BaselineVersion)) -and (-not [string]::IsNullOrEmpty($BaselineSourceRef))) {
    throw "-BaselineVersion and -BaselineSourceRef are mutually exclusive."

[Copilot]

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.

Suppressed comments (1)

eng/pipelines/perf/README.md:270

• The README states that the switch-experiment baseline requires “no second build at all”, but the scripts only reuse a single build in the interleaved path. In sequential mode, run-perf-tests.sh/.ps1 still run the baseline and current passes separately (and therefore build twice). Clarifying this avoids misleading users who select sequential mode.

- **Baseline (switch experiment)**: no second build at all — `--switch-under-test` measures one
  source tree twice, so the scripts build the `current` variant once and point both passes at it,
  differing only in the runner config each pass is handed. Used by the experiment pipeline.

[Copilot]

Pull request overview

Copilot reviewed 17 out of 17 changed files in this pull request and generated 1 comment.

Suppressed comments (1)

src/Microsoft.Data.SqlClient/tests/PerformanceTests/BenchmarkRunners/ConnectionPoolContentionRunner.cs:127

• result is assigned but never used (var result = cmd.ExecuteScalar();). With TreatWarningsAsErrors enabled, this will fail the build with CS0219. Use a discard assignment (or otherwise consume the value) to keep the call but avoid the unused-local warning.

                        var result = cmd.ExecuteScalar();

[Copilot]

Pull request overview

Copilot reviewed 17 out of 17 changed files in this pull request and generated no new comments.

Suppressed comments (1)

src/Microsoft.Data.SqlClient/tests/PerformanceTests/BenchmarkRunners/ConnectionPoolContentionRunner.cs:127

• result is assigned but never used. This produces CS0219 (treated as error in this repo) and will break the build. If the intention is just to prevent the JIT from eliding the call, discard the value explicitly as elsewhere in this file (_ = ...).

                        var result = cmd.ExecuteScalar();

[Copilot]

Pull request overview

Copilot reviewed 17 out of 17 changed files in this pull request and generated no new comments.

Suppressed comments (2)

src/Microsoft.Data.SqlClient/tests/PerformanceTests/BenchmarkRunners/ConnectionPoolContentionRunner.cs:128

• var result = cmd.ExecuteScalar(); assigns to a local that is never used. With TreatWarningsAsErrors=true, this will fail the build (unused variable warning). Use a discard (_ = ...) like the dedicated-thread variant does.

                        using var conn = new SqlConnection(_connectionString);
                        conn.Open();
                        using var cmd = conn.CreateCommand();
                        cmd.CommandText = "SELECT 1";
                        var result = cmd.ExecuteScalar();
                        // Dispose returns the connection to the pool.

eng/pipelines/perf/scripts/interleave_perf.py:223

• On non-Windows platforms, _kill_tree only terminates the direct benchmark process (proc.terminate() / proc.kill()), so any child processes can remain alive. That contradicts the function’s intent (“and any children”) and risks leaving orphaned processes holding SQL connections, which could skew subsequent units. Consider launching benchmark subprocesses in their own process group/session and then killing the whole group (e.g., start_new_session=True / setsid + os.killpg).

        else:
            proc.terminate()
            try:
                proc.wait(timeout=30)
            except subprocess.TimeoutExpired:

[Copilot]

Pull request overview

Copilot reviewed 17 out of 17 changed files in this pull request and generated no new comments.

Suppressed comments (2)

Previously missed (1) — in code that hasn't changed since the last review.

eng/pipelines/perf/scripts/interleave_perf.py:205

• If a benchmark unit exceeds the timeout, run_unit_process attempts to kill the process tree but then calls proc.wait() with no timeout. If the kill fails (e.g., taskkill permission issue on Windows), this can hang indefinitely and defeat the whole point of the per-unit backstop. Consider adding a bounded wait after killing and returning a non-zero code if the process still doesn’t exit promptly.

        try:
            rc = proc.wait(timeout=timeout_secs if timeout_secs > 0 else None)
        except subprocess.TimeoutExpired:
            timed_out = True
            print(f"ERROR: unit '{unit}' exceeded the per-unit timeout of {timeout_secs}s; "
                  f"killing pid {proc.pid}.", file=sys.stderr)
            _kill_tree(proc)
            rc = proc.wait()
    return rc, time.monotonic() - started, affinity_note, timed_out

src/Microsoft.Data.SqlClient/tests/PerformanceTests/BenchmarkRunners/ConnectionPoolContentionRunner.cs:128

• cmd.ExecuteScalar() is assigned to a local (result) that is never used. With TreatWarningsAsErrors enabled, this can fail the build with CS0219 (assigned but never used). Use a discard assignment (or just call ExecuteScalar() without storing) instead.

                        using var conn = new SqlConnection(_connectionString);
                        conn.Open();
                        using var cmd = conn.CreateCommand();
                        cmd.CommandText = "SELECT 1";
                        var result = cmd.ExecuteScalar();
                        // Dispose returns the connection to the pool.

[Copilot]

Pull request overview

Copilot reviewed 17 out of 17 changed files in this pull request and generated 1 comment.

Suppressed comments (6)

Previously missed (2) — in code that hasn't changed since the last review.

eng/pipelines/perf/scripts/interleave_perf.py:195

• On non-Windows, _kill_tree currently only terminates the direct child, but its docstring says it kills children too. If a benchmark unit times out and leaves child processes running, they can keep SQL connections open and skew subsequent units. Consider starting each benchmark subprocess in its own process group/session so the timeout path can reliably terminate the whole tree.

    with open(log_path, "w", encoding="utf-8") as log:
        proc = subprocess.Popen(cmd, cwd=cwd, env=env, stdout=log,
                                stderr=subprocess.STDOUT)

src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTest.cs:274

• This test checks out internalConnection on the inline fast path but never returns it to the pool, leaving the pool in a "connection checked out" state for the remainder of the test. Returning it makes the test self-contained and avoids relying on finalizer/emancipated-connection cleanup.

This issue also appears in the following locations of the same file:

• line 288
• line 311
• line 320

            TaskCompletionSource<DbConnectionInternal> taskCompletionSource = new();
            var completed = pool.TryGetConnection(
                new SqlConnection(),
                taskCompletionSource,
                TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)),

eng/pipelines/perf/scripts/interleave_perf.py:223

• _kill_tree claims to kill the process tree, but on non-Windows it only calls terminate()/kill() on the direct child. With the per-unit timeout feature, leaving orphaned children behind is especially risky because they can continue running benchmarks or hold SQL connections after the orchestrator moves on. If run_unit_process starts the subprocess in its own session (e.g., start_new_session=True), _kill_tree can kill the whole process group on Unix.

        else:
            proc.terminate()
            try:
                proc.wait(timeout=30)
            except subprocess.TimeoutExpired:

src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTest.cs:288

• After asserting the inline hit, return the checked-out connection to the pool so later assertions (and any subsequent tests) don't observe a leaked checkout.

            Assert.Equal(1, pool.Count);

src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTest.cs:317

• This test checks out an internal connection but never returns it to the pool. Returning it keeps the test isolated and avoids depending on reclamation/finalization behavior.

            // Act
            var completed = pool.TryGetConnection(
                new SqlConnection(),
                taskCompletionSource: null,
                TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)),
                out DbConnectionInternal? internalConnection
            );

src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTest.cs:322

• Return the checked-out connection to the pool before leaving the test so the pool doesn't retain a busy connection beyond the assertion scope.

            Assert.True(completed);
            Assert.Equal(pooledConnection, internalConnection);
            Assert.Equal(1, pool.Count);

[Copilot]

Pull request overview

Copilot reviewed 17 out of 17 changed files in this pull request and generated 1 comment.

Suppressed comments (2)

src/Microsoft.Data.SqlClient/tests/PerformanceTests/BenchmarkRunners/ConnectionPoolContentionRunner.cs:128

• SteadyStateOpenQueryClose assigns cmd.ExecuteScalar() to a local (result) that is never used. This produces CS0219 (“assigned but its value is never used”), and the repo builds with warnings-as-errors, so the PerformanceTests project will fail to compile.

                        using var conn = new SqlConnection(_connectionString);
                        conn.Open();
                        using var cmd = conn.CreateCommand();
                        cmd.CommandText = "SELECT 1";
                        var result = cmd.ExecuteScalar();
                        // Dispose returns the connection to the pool.

eng/pipelines/perf/scripts/interleave_perf.py:224

• _kill_tree() claims to kill the process “and any children”, but on non-Windows it only terminates the direct process (proc.terminate()/proc.kill()) and does not actually kill the process tree. This mismatch is misleading when a timed-out unit leaves child processes running (the exact scenario the comment describes).

def _kill_tree(proc):
    """Kill *proc* and any children.  Best effort; never raises.

    The benchmark host can leave orphaned child processes holding SQL connections, which would
    keep occupying the pool and skew whatever runs next, so kill the tree rather than just the
    direct child.
    """
    try:
        if os.name == "nt":
            subprocess.run(["taskkill", "/F", "/T", "/PID", str(proc.pid)],
                           stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL, check=False)
        else:
            proc.terminate()
            try:
                proc.wait(timeout=30)
            except subprocess.TimeoutExpired:
                proc.kill()

[Copilot]

Pull request overview

Copilot reviewed 17 out of 17 changed files in this pull request and generated no new comments.

Suppressed comments (2)

src/Microsoft.Data.SqlClient/tests/PerformanceTests/BenchmarkRunners/ConnectionPoolContentionRunner.cs:128

• result is assigned but never used. Because the repo builds with TreatWarningsAsErrors=true, this produces CS0219 and fails the build. Use a discard assignment (or otherwise consume the value) instead of introducing an unused local.

                        using var conn = new SqlConnection(_connectionString);
                        conn.Open();
                        using var cmd = conn.CreateCommand();
                        cmd.CommandText = "SELECT 1";
                        var result = cmd.ExecuteScalar();
                        // Dispose returns the connection to the pool.

eng/pipelines/perf/scripts/run-perf-tests.ps1:169

• The warning message uses the switch name (e.g. -UseConnectionPoolV2) but implies that parameter itself is ignored ("-$SwitchUnderTest is ignored..."). What’s actually ignored is the corresponding -Use* override provided alongside -SwitchUnderTest. Update the warning to name the ignored -Use* parameter so the guidance is accurate.

# -SwitchUnderTest forces its switch explicitly for each pass (baseline=false, current=true), so a
# separately-supplied -Use* flag for that SAME switch would be silently overridden; warn rather than
# let that go unnoticed.  Other -Use* flags still apply normally to both passes.
$conflictingFlagValue = switch ($SwitchUnderTest) {
    "UseConnectionPoolV2"        { $UseConnectionPoolV2 }
    "UseOptimizedAsyncBehaviour" { $UseOptimizedAsyncBehaviour }
    "UseManagedSniOnWindows"     { $UseManagedSniOnWindows }
    default                      { "" }
}
if (-not [string]::IsNullOrEmpty($conflictingFlagValue)) {
    Write-Warning "-$SwitchUnderTest is ignored when -SwitchUnderTest is $SwitchUnderTest (baseline forces false, current forces true)."
}

[Copilot]

Pull request overview

Copilot reviewed 16 out of 16 changed files in this pull request and generated no new comments.

Suppressed comments (1)

src/Microsoft.Data.SqlClient/tests/PerformanceTests/BenchmarkRunners/ConnectionPoolContentionRunner.cs:128

• This local is assigned but never used (var result = cmd.ExecuteScalar();). In this repo warnings are treated as errors, so this will fail the build with CS0219. If the intent is only to execute the query, keep the discard assignment.

                        using var conn = new SqlConnection(_connectionString);
                        conn.Open();
                        using var cmd = conn.CreateCommand();
                        cmd.CommandText = "SELECT 1";
                        var result = cmd.ExecuteScalar();
                        // Dispose returns the connection to the pool.

[Copilot]

Pull request overview

Copilot reviewed 16 out of 16 changed files in this pull request and generated no new comments.

Suppressed comments (1)

src/Microsoft.Data.SqlClient/tests/PerformanceTests/BenchmarkRunners/ConnectionPoolContentionRunner.cs:127

• result is assigned but never used. With TreatWarningsAsErrors enabled in this repo, this will produce CS0219 and fail the build. If the intention is only to execute the query, keep the call but don’t store it in an unused local.

                        var result = cmd.ExecuteScalar();

[Copilot]

Pull request overview

Copilot reviewed 16 out of 16 changed files in this pull request and generated no new comments.

Suppressed comments (2)

src/Microsoft.Data.SqlClient/tests/PerformanceTests/BenchmarkRunners/ConnectionPoolContentionRunner.cs:128

• The local variable result is assigned but never used (var result = cmd.ExecuteScalar();). With TreatWarningsAsErrors enabled in this repo, this will trigger CS0219 and fail the build. If the intent is simply to execute the query, discard the result instead.

                        using var conn = new SqlConnection(_connectionString);
                        conn.Open();
                        using var cmd = conn.CreateCommand();
                        cmd.CommandText = "SELECT 1";
                        var result = cmd.ExecuteScalar();
                        // Dispose returns the connection to the pool.

src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs:1796

• using System.Runtime.CompilerServices; appears to now be unused after replacing the prior ConfiguredValueTaskAwaitable/awaiter-based blocking with ReadAsync(...).AsTask().GetAwaiter().GetResult(). With TreatWarningsAsErrors, this will raise CS8019 and break the build; please remove the unused using directive.

            }
            catch
            {
                // At this point, the connection is "out of the pool" (the call to postpop). If we hit a transient
                // error anywhere along the way when enlisting the connection in the transaction, we need to get
                // the connection back into the pool so that it isn't leaked.
                ReturnInternalConnection(connection, owningObject);
                throw;

[Copilot]

Pull request overview

Copilot reviewed 16 out of 16 changed files in this pull request and generated no new comments.

Suppressed comments (1)

src/Microsoft.Data.SqlClient/tests/PerformanceTests/BenchmarkRunners/ConnectionPoolContentionRunner.cs:127

• var result = cmd.ExecuteScalar(); assigns to a local that is never used, which will raise CS0219 and fail the build when warnings are treated as errors. The result doesn’t need to be captured to ensure the query executes; use a discard (or just call the method) instead.

                        var result = cmd.ExecuteScalar();

[mdaigle]

🤖 Reviewed the benchmark and fast-path test changes.

[Copilot]

Pull request overview

Copilot reviewed 16 out of 16 changed files in this pull request and generated no new comments.

[mdaigle]

🤖 Re-reviewed the latest updates.

[Copilot]

Pull request overview

Copilot reviewed 16 out of 16 changed files in this pull request and generated no new comments.

Suppressed comments (2)

src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs:1746

• ReadChannelSyncOverAsync now blocks on ReadAsync(...).AsTask().GetAwaiter().GetResult() with no sync-over-async throttle. On net462 (still a supported TFM), blocking thread-pool threads does not trigger the cooperative-blocking compensation that modern .NET relies on, so the channel completion can deadlock under saturation/thread-pool starvation (the exact scenario the removed semaphore previously guarded). Consider restoring a sync-over-async gate for NETFRAMEWORK to keep at least one worker available to run queued channel completions.

            // Channel has no blocking read. Block on the Task rather than an opaque primitive: the
            // idle channel is created without AllowSynchronousContinuations, so the completing
            // continuation is queued, and Task blocking lets the thread pool inject a worker to run it.
            return _idleChannel.ReadAsync(cancellationToken).AsTask().GetAwaiter().GetResult();
        }

src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs:68

• The sync-over-async starvation protection semaphore was removed from the pool. If ReadChannelSyncOverAsync is guarded under NETFRAMEWORK (see related comment), the pool still needs a NETFRAMEWORK-only static SemaphoreSlim field to compile and to preserve the original deadlock-avoidance behavior for net462 callers that block on channel waits from thread-pool threads.

This issue also appears on line 1742 of the same file.

        #region Fields
        /// <summary>
        /// Tracks the number of instances of this class. Used to generate unique IDs for each instance.
        /// </summary>
        private static int _instanceCount;

[cheenamalhotra]

Please validate this saturated synchronous path on net462. This implementation relies on cooperative thread-pool blocking, which .NET Framework lacks. Add a targeted saturation test and use a framework-specific strategy if it shows a regression.

[cheenamalhotra]

Please derive the control from Parallelism too. On small hosts, ProcessorCount * 8 can remain below 50, so every case is still starved and the benchmark has no non-starved control.

[cheenamalhotra]

Please reject UseManagedSniOnWindows when platform=linux. It currently runs a full experiment with no behavior change and can report a misleading ~0% delta.