From b914e210465b87203b298ad40763e7fbabb23c92 Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Thu, 13 Aug 2026 14:23:38 -0700 Subject: [PATCH 01/34] Add perf switch experiment pipeline Adds a third perf pipeline that A/B tests one runner-config switch against itself on the same source build, alongside the existing package-baseline and PR-baseline pipelines. The run scripts gain a general --switch-under-test / -SwitchUnderTest option (UseConnectionPoolV2, UseOptimizedAsyncBehaviour, UseManagedSniOnWindows) that writes two runner configs differing only in that key and hands one to each pass. These are AppContext switches latched process-wide, so they cannot be toggled between benchmarks in a single process. Both passes share one build, and the option is rejected alongside a source baseline so the delta stays attributable to one variable. The new pipeline is a separate file rather than a flag on the other two so that skipping Kusto is structural: both rows would share a DerivedRunId, PerfRun.Config is stamped once per run, and nothing marks a row as an experiment. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- eng/pipelines/perf/README.md | 80 +++++- eng/pipelines/perf/scripts/interleave_perf.py | 35 ++- eng/pipelines/perf/scripts/run-perf-tests.ps1 | 117 +++++++-- eng/pipelines/perf/scripts/run-perf-tests.sh | 124 ++++++++- .../perf/sqlclient-perf-switch-pipeline.yml | 235 ++++++++++++++++++ 5 files changed, 556 insertions(+), 35 deletions(-) create mode 100644 eng/pipelines/perf/sqlclient-perf-switch-pipeline.yml diff --git a/eng/pipelines/perf/README.md b/eng/pipelines/perf/README.md index 09ad48788f..0243a84f99 100644 --- a/eng/pipelines/perf/README.md +++ b/eng/pipelines/perf/README.md @@ -12,7 +12,8 @@ database. | ---- | ------- | | `sqlclient-perf-pipeline.yml` | The main (manual/nightly) pipeline. Extends `v1/Perf.Test.Job.yml@PerfTemplates`. Baseline = released NuGet package; ingests into Kusto. | | `sqlclient-perf-pr-pipeline.yml` | PR pipeline. Same template, same scripts, same options; baseline = **`main` branch source**; **no Kusto ingestion**. | -| `scripts/run-perf-tests.sh` | Linux on-VM entry point: install SDK, create DB, run benchmarks (interleaved or sequential), compare. Baseline is either a released package (`--baseline-version`) or another git ref's source (`--baseline-source-ref`). | +| `sqlclient-perf-switch-pipeline.yml` | Switch experiment pipeline. Same template, same scripts; both passes build the **same source** and differ only in one runner-config switch; **no Kusto ingestion**. | +| `scripts/run-perf-tests.sh` | Linux on-VM entry point: install SDK, create DB, run benchmarks (interleaved or sequential), compare. Baseline is a released package (`--baseline-version`), another git ref's source (`--baseline-source-ref`), or the same source with one runner-config switch flipped off (`--switch-under-test`). | | `scripts/run-perf-tests.ps1` | Windows equivalent (ProcessorAffinity instead of `taskset`). | | `scripts/interleave_perf.py` | Interleaved + best-of-N orchestrator: runs each unit baseline↔candidate back-to-back and confirms regressions across N passes. | | `scripts/compare_perf.py` | Compares baseline vs current BenchmarkDotNet JSON → delta (md + json). Reused by the orchestrator. | @@ -151,6 +152,66 @@ the comparison (and one removed by the PR as `removed`) instead of failing the r share the single generated runner config (`RUNNER_CONFIG` / `DATATYPES_CONFIG` env vars), so connection string and behaviour flags are identical on both sides. +## Switch experiment pipeline (`sqlclient-perf-switch-pipeline.yml`) + +The three perf pipelines are the same benchmarks, template and scripts pointed at three different +questions. Two of them vary the **source** under measurement; the third varies the **config**: + +| Question | Pipeline | Baseline | Current | +| --- | --- | --- | --- | +| Has this branch regressed against a released package? | `sqlclient-perf-pipeline.yml` | released NuGet package | queued branch | +| Does my PR regress the branch it merges into? | `sqlclient-perf-pr-pipeline.yml` | `main` source | queued branch | +| What does this switch cost or buy? | `sqlclient-perf-switch-pipeline.yml` | queued branch, switch **off** | queued branch, switch **on** | + +`sqlclient-perf-switch-pipeline.yml` picks one runner-config switch via the `switchUnderTest` +queue-time parameter (`UseConnectionPoolV2`, `UseOptimizedAsyncBehaviour` or +`UseManagedSniOnWindows`) and runs the baseline pass with it `false` and the current pass with it +`true`. Both passes measure the **same commit** — the branch the run is queued on — so queue it on a +PR branch to ask "what does this switch do to my change?", or on `main` to ask "what does it do to +`main`?". + +Two passes are required because these are `AppContext` switches latched process-wide (for example +`UseConnectionPoolV2` is read and cached the first time a connection pool is created), so they cannot +be toggled between benchmarks within a single process. The pipeline wires that into the existing +comparison machinery — interleaved best-of-N or sequential, via `benchmarkRunMode` — instead of two +ad hoc manual runs. + +Switch-pipeline parameters that differ from the tables above: + +| Parameter | Default | Description | +| --------- | ------- | ----------- | +| `switchUnderTest` | `UseConnectionPoolV2` | The single switch to A/B. Baseline forces it `false`, current forces it `true`. | +| `failIfSwitchSlower` | `false` | Maps onto the scripts' `--fail-on-regression` gate, but means something different here: "switch on is slower" is usually the *result* you queued the run to measure, not a defect. Enable it only when asserting the switch must not be a slowdown (e.g. before flipping its default). | +| `testTimeoutMinutes` | `180` | Same as the main pipeline, not the PR pipeline's `210`: both sides are the same source, so only **one** driver build is needed. | + +The pipeline deliberately does **not** expose `baselineVersion` / `baselineSourceRef` (the run +scripts reject combining those with `--switch-under-test`, since a simultaneous source change would +make the delta unattributable), nor the `useManagedSniOnWindows` / `useOptimizedAsyncBehaviour` / +`useConnectionPoolV2` flags. Every switch except the one under test stays at its checked-in +`runnerconfig.jsonc` value, so the measured difference is attributable to exactly one variable. + +### Why these runs are never ingested into Kusto + +This mode has its own pipeline file, rather than being a flag on the other two, specifically so that +"never ingested" is structural rather than a conditional someone can flip. Ingesting a switch +experiment would corrupt the perf database three ways: + +* **`DerivedRunId` collision.** The ID is `driver|commit|pipelineRunId`. The other two pipelines keep + their two rows distinct because the baseline row carries a *different* commit (`v7.0.2`, or the + baseline ref's sha). Here both passes are the same commit in the same pipeline run, so both rows + would derive the same ID. +* **`PerfRun.Config` is stamped once per run.** `translate_results_to_kusto.sh` builds one + `--config-override` set from the queue-time `CFG_*` values and reuses it for both the baseline and + current rows. That is correct when the config genuinely is shared, but it means the two rows could + not record the differing switch values that are the entire point of the experiment. +* **Trend pollution.** No field marks a row as an experiment — `RunType` is already + `Sequential`/`Interweaved` — so the switch-on pass would be indistinguishable from an ordinary + measurement of the branch and would distort the very trends the other two pipelines exist to + protect. + +The comparison report and the raw BenchmarkDotNet artifacts are published as usual, and the build is +tagged `Switch ` so experiments are identifiable in the ADO build list. + ## Two-pass build model The `PerformanceTests` project references Microsoft.Data.SqlClient two ways, selected by MSBuild: @@ -162,6 +223,9 @@ The `PerformanceTests` project references Microsoft.Data.SqlClient two ways, sel - **Baseline (source)**: no reference switching at all — the baseline ref's own copy of the perf project is built from `../sqlclient-perf-baseline-src`, keeping its default `ProjectReference` to that ref's driver source. Used by the PR pipeline. +- **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 switch pipeline. The VM's `NuGet.config` exposes only the governed feed, and CPM rejects multiple unmapped sources (`NU1507`). The baseline pass therefore restores through a **dedicated single-source config** @@ -323,6 +387,18 @@ translated NDJSON as the `perf-kusto-payloads` artifact for manual/backfill inge 3. After the run, review the **run summary** (comparison, labelled `@`) and the `perf-results` artifact. The build is tagged **`Baseline `**. +### Running the switch pipeline + +1. Open the **switch** performance test pipeline (`sqlclient-perf-switch-pipeline.yml`) in Azure + DevOps and select **Run pipeline**. +2. Choose the branch whose behaviour you want to measure — `main` to characterise the switch on its + own, or a PR branch to characterise it against that change — and pick `switchUnderTest`. There is + no baseline selector: the baseline *is* this branch with the switch off. No Kusto configuration is + involved; these results are never ingested (see [Why these runs are never ingested into + Kusto](#why-these-runs-are-never-ingested-into-kusto)). +3. After the run, review the **run summary** (comparison, labelled `=false`) and the + `perf-results` artifact. The build is tagged **`Switch `**. + ## Troubleshooting | Symptom | Likely cause / fix | @@ -331,6 +407,8 @@ translated NDJSON as the `perf-kusto-payloads` artifact for manual/backfill inge | Baseline restore fails to find MDS | `baselineVersion` isn't a published NuGet.org version, or the VM has no outbound access to `api.nuget.org`. | | Baseline pass fails to compile: `CS1061 ... does not contain a definition for ` | A benchmark calls an MDS API newer than `baselineVersion`. Guard it with the `MDS_GE_` constants and add an older fallback — see [Benchmarks must compile against the oldest baseline](#benchmarks-must-compile-against-the-oldest-baseline). | | No comparison / summary | The baseline pass was skipped (empty `baselineVersion` / `baselineSourceRef`) or one pass produced no `*-report-full.json`. | +| `--switch-under-test is mutually exclusive with --baseline-version and --baseline-source-ref` | A switch experiment was combined with a source baseline. The switch pipeline never does this; if you are invoking the scripts directly, clear the baseline selector — varying source and config at once makes the delta unattributable. | +| Switch experiment shows a ~0% delta everywhere | Expected for benchmarks the switch does not touch. If *every* benchmark is flat, check the run log's `Switch A/B` line actually names the switch, and that the switch is one the driver reads at startup via the runner config. | | Baseline source ref not found (PR pipeline) | `baselineSourceRef` is not a branch on `origin`, and the fallback `git clone --branch ` of `baselineRepoUrl` also failed (ref does not exist there, or the VM has no outbound access to the remote). The reason git gave is echoed into the build log and saved to `/diagnostics/git-*.log`. | | `Fetching baseline ref ... from the checkout's origin` is the last line, then the job stalls | Should no longer happen. The checkout is copied to the VM without credentials (ADO's checkout task defaults to `persistCredentials: false`), so a fetch from an authenticated `origin` used to sit on a `Username for ...` prompt forever. All network git calls now run with `GIT_TERMINAL_PROMPT=0`, no credential helper, stdin closed, and a `GIT_NET_TIMEOUT_SECS` (default 300s) hard timeout, so this fails in under a second and falls back to cloning `baselineRepoUrl`. A fetch failure here is expected and harmless on ADO. | | Ingestion step skipped | `enableKustoIngestion` is `false`, or `KustoClusterUri`, `KustoDatabase` or `KustoServiceConnection` (from `ADX Cluster Variables`) is empty (expected until the cluster is provisioned). | diff --git a/eng/pipelines/perf/scripts/interleave_perf.py b/eng/pipelines/perf/scripts/interleave_perf.py index 167002fb8f..6522adced1 100644 --- a/eng/pipelines/perf/scripts/interleave_perf.py +++ b/eng/pipelines/perf/scripts/interleave_perf.py @@ -101,17 +101,22 @@ def apply_affinity(proc, cpus): # -------------------------------------------------------------------------------------------------- # Running one unit and collecting its artifacts. # -------------------------------------------------------------------------------------------------- -def run_unit_process(exe_dir, assembly, unit, cwd, cpus, log_path): +def run_unit_process(exe_dir, assembly, unit, cwd, cpus, log_path, env_overrides=None): """Run one benchmark *unit* from the build at *exe_dir* in *cwd*. Returns the subprocess return code. Kept as a small seam so tests can substitute - a fake runner. + a fake runner. *env_overrides*, when given, is applied on top of the inherited + environment (e.g. a per-variant RUNNER_CONFIG so baseline and current can run with + different SqlClient behaviour flags, such as comparing the legacy vs new connection + pool from the SAME build). """ os.makedirs(cwd, exist_ok=True) cmd = ["dotnet", os.path.join(exe_dir, assembly)] env = dict(os.environ) env["PERF_BENCHMARK"] = unit env.pop("PERF_LIST_BENCHMARKS", None) + if env_overrides: + env.update(env_overrides) with open(log_path, "w", encoding="utf-8") as log: proc = subprocess.Popen(cmd, cwd=cwd, env=env, stdout=log, @@ -154,12 +159,18 @@ def collect_results(cwd, dest): class Runner: """Holds the invariant run parameters and performs interleaved unit passes.""" - def __init__(self, baseline_dir, current_dir, assembly, work_dir, cpus): + def __init__(self, baseline_dir, current_dir, assembly, work_dir, cpus, + baseline_runner_config=None, current_runner_config=None): self.baseline_dir = baseline_dir self.current_dir = current_dir self.assembly = assembly self.work_dir = work_dir self.cpus = cpus + # Optional per-variant RUNNER_CONFIG override (e.g. --switch-under-test needs baseline + # and current to run with different SqlClient behaviour flags even though they share the + # same build). None means "no override" -> both variants use the ambient RUNNER_CONFIG. + self.baseline_runner_config = baseline_runner_config + self.current_runner_config = current_runner_config def list_units(self): cmd = ["dotnet", os.path.join(self.current_dir, self.assembly)] @@ -175,7 +186,11 @@ def _run_one(self, variant, exe_dir, unit, rep, agg_dir): shutil.rmtree(cwd) os.makedirs(cwd, exist_ok=True) log_path = os.path.join(cwd, "run.log") - rc = run_unit_process(exe_dir, self.assembly, unit, cwd, self.cpus, log_path) + runner_config = (self.baseline_runner_config if variant == "baseline" + else self.current_runner_config) + env_overrides = {"RUNNER_CONFIG": runner_config} if runner_config else None + rc = run_unit_process(exe_dir, self.assembly, unit, cwd, self.cpus, log_path, + env_overrides=env_overrides) if rc != 0: _tail(log_path) raise RuntimeError(f"benchmark unit '{unit}' ({variant}, rep {rep}) failed (exit {rc}).") @@ -353,6 +368,14 @@ def main(argv=None): parser.add_argument("--reps", type=int, default=3, help="Total interleaved passes for a flagged unit (best-of-N). 1 disables confirmation.") parser.add_argument("--baseline-version", default="baseline") + parser.add_argument("--baseline-runner-config", default=None, + help="Override RUNNER_CONFIG for baseline-variant subprocesses only " + "(e.g. to force a different SqlClient behaviour flag for the " + "baseline, such as comparing the legacy vs new connection pool " + "from the same build). Omit to use the ambient RUNNER_CONFIG for " + "both variants (default behaviour).") + parser.add_argument("--current-runner-config", default=None, + help="Override RUNNER_CONFIG for current-variant subprocesses only.") parser.add_argument("--client-cpus", default=os.environ.get("PERF_CLIENT_CPUS", ""), help="CPU set to pin the benchmark client to, e.g. '16-31'.") parser.add_argument("--fail-on-regression", action="store_true", @@ -376,7 +399,9 @@ def main(argv=None): runner = Runner(os.path.abspath(args.baseline_exe_dir), os.path.abspath(args.current_exe_dir), - args.assembly, work_dir, cpus) + args.assembly, work_dir, cpus, + baseline_runner_config=args.baseline_runner_config, + current_runner_config=args.current_runner_config) units = runner.list_units() if not units: diff --git a/eng/pipelines/perf/scripts/run-perf-tests.ps1 b/eng/pipelines/perf/scripts/run-perf-tests.ps1 index 7ea2bfdffe..bf740e61eb 100644 --- a/eng/pipelines/perf/scripts/run-perf-tests.ps1 +++ b/eng/pipelines/perf/scripts/run-perf-tests.ps1 @@ -54,7 +54,17 @@ param( [ValidateSet("", "true", "false")] [string]$UseOptimizedAsyncBehaviour = "", [ValidateSet("", "true", "false")] - [string]$UseConnectionPoolV2 = "" + [string]$UseConnectionPoolV2 = "", + # Alternative to -BaselineVersion/-BaselineSourceRef: an A/B experiment on ONE runner-config + # switch. Both passes build the SAME source; only the named switch differs (baseline=false, + # current=true), which is the only way to compare a switch whose value is latched process-wide + # (e.g. UseConnectionPoolV2 is read and cached the first time a pool is created). Mutually + # exclusive with the other two baseline selectors, and overrides the matching -Use* flag (which + # would otherwise be ambiguous: one value cannot describe two passes). ValidateSet restricts it + # to switches this script knows how to stamp, so a typo fails fast at binding time instead of + # silently writing an inert key and reporting a meaningless zero-delta comparison. + [ValidateSet("", "UseConnectionPoolV2", "UseOptimizedAsyncBehaviour", "UseManagedSniOnWindows")] + [string]$SwitchUnderTest = "" ) $ErrorActionPreference = "Stop" @@ -125,6 +135,7 @@ Write-Host " Results dir : $ResultsDir" Write-Host " Run mode : $RunMode (confirmation runs: $ConfirmationRuns)" Write-Host " Baseline ver : $(if ($BaselineVersion) { $BaselineVersion } else { '' })" Write-Host " Baseline ref : $(if ($BaselineSourceRef) { $BaselineSourceRef } else { '' })" +Write-Host " Switch A/B : $(if ($SwitchUnderTest) { "$SwitchUnderTest (baseline=false vs current=true)" } else { '' })" Write-Host " SQL_SERVER : $SqlServer" Write-Host " PERF_CLIENT_CPUS: $($env:PERF_CLIENT_CPUS)" Write-Host " PERF_SQL_CPUS : $($env:PERF_SQL_CPUS)" @@ -138,6 +149,21 @@ if (-not (Test-Path $PerfProject)) { if ((-not [string]::IsNullOrEmpty($BaselineVersion)) -and (-not [string]::IsNullOrEmpty($BaselineSourceRef))) { throw "-BaselineVersion and -BaselineSourceRef are mutually exclusive." } +if ((-not [string]::IsNullOrEmpty($SwitchUnderTest)) -and ((-not [string]::IsNullOrEmpty($BaselineVersion)) -or (-not [string]::IsNullOrEmpty($BaselineSourceRef)))) { + throw "-SwitchUnderTest is mutually exclusive with -BaselineVersion and -BaselineSourceRef: it compares the SAME source build with one switch flipped, so mixing in a source change would make the delta unattributable." +} +# -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)." +} if ([string]::IsNullOrEmpty($SqlPassword)) { throw "SQL_PASSWORD environment variable is not set (expected from the perf template)." } @@ -299,20 +325,11 @@ $env:RUNNER_CONFIG = $RunnerConfig # It needs no per-run modification, so point the env var at the checked-in file directly. $env:DATATYPES_CONFIG = Join-Path $PerfDir "datatypes.json" -$srcConfig = Join-Path $PerfDir "runnerconfig.jsonc" -$rawConfig = Get-Content $srcConfig -Raw -# Strip // line comments so ConvertFrom-Json accepts the .jsonc content. -$rawConfig = ($rawConfig -split "`n" | ForEach-Object { $_ -replace '(?m)^\s*//.*$', '' }) -join "`n" -$cfg = ConvertFrom-Json $rawConfig - # SqlClient connection-string values may be wrapped in double quotes; doubling any embedded double # quote lets a password containing ';', '=', spaces or single quotes be parsed as a single literal # value instead of corrupting the connection string. $escapedPassword = '"' + ($SqlPassword -replace '"', '""') + '"' -$cfg.ConnectionString = "Server=tcp:$SqlServer,1433;User ID=sa;Password=$escapedPassword;Initial Catalog=$DbName;TrustServerCertificate=True;Encrypt=False;" -# Apply the optional SqlClient behaviour overrides supplied by the pipeline. An empty value leaves -# the checked-in default untouched; otherwise the flag is forced to the requested boolean so the -# benchmarks run with (and PerfRun.Config records) exactly the requested behaviour. + function Set-CfgBool { param($Config, [string]$Name, [string]$Value) if (-not [string]::IsNullOrEmpty($Value)) { @@ -321,11 +338,51 @@ function Set-CfgBool { else { $Config | Add-Member -NotePropertyName $Name -NotePropertyValue $b } } } -Set-CfgBool $cfg "UseManagedSniOnWindows" $UseManagedSniOnWindows -Set-CfgBool $cfg "UseOptimizedAsyncBehaviour" $UseOptimizedAsyncBehaviour -Set-CfgBool $cfg "UseConnectionPoolV2" $UseConnectionPoolV2 -$cfg | ConvertTo-Json -Depth 10 | Set-Content -Path $RunnerConfig -Encoding UTF8 -Write-Host "Wrote runner config to $RunnerConfig (Server=tcp:$SqlServer,1433; Initial Catalog=$DbName)" + +# Write-RunnerConfig [switchName] [switchValue] +# Writes one runner config (checked-in runnerconfig.jsonc + injected connection string + behaviour +# overrides) to . When is given, that config key is forced to +# ("true"/"false") regardless of the corresponding -Use* parameter -- used by -SwitchUnderTest, which +# needs a different value for the same switch in each pass. With no switch name the config is built +# purely from the -Use* parameters, exactly as before. +function Write-RunnerConfig { + param([string]$Dst, [string]$SwitchName = "", [string]$SwitchValue = "") + $srcConfig = Join-Path $PerfDir "runnerconfig.jsonc" + $rawConfig = Get-Content $srcConfig -Raw + # Strip // line comments so ConvertFrom-Json accepts the .jsonc content. + $rawConfig = ($rawConfig -split "`n" | ForEach-Object { $_ -replace '(?m)^\s*//.*$', '' }) -join "`n" + $cfg = ConvertFrom-Json $rawConfig + + $cfg.ConnectionString = "Server=tcp:$SqlServer,1433;User ID=sa;Password=$escapedPassword;Initial Catalog=$DbName;TrustServerCertificate=True;Encrypt=False;" + # Apply the optional SqlClient behaviour overrides supplied by the pipeline. An empty value + # leaves the checked-in default untouched; otherwise the flag is forced to the requested boolean + # so the benchmarks run with (and PerfRun.Config records) exactly the requested behaviour. The + # switch-under-test override (when this config names one) takes precedence over the matching + # -Use* parameter, so a single checked-in template can be stamped out per pass with that one + # switch flipped and everything else identical. + $sniValue = if ($SwitchName -eq "UseManagedSniOnWindows") { $SwitchValue } else { $UseManagedSniOnWindows } + $asyncValue = if ($SwitchName -eq "UseOptimizedAsyncBehaviour") { $SwitchValue } else { $UseOptimizedAsyncBehaviour } + $poolValue = if ($SwitchName -eq "UseConnectionPoolV2") { $SwitchValue } else { $UseConnectionPoolV2 } + Set-CfgBool $cfg "UseManagedSniOnWindows" $sniValue + Set-CfgBool $cfg "UseOptimizedAsyncBehaviour" $asyncValue + Set-CfgBool $cfg "UseConnectionPoolV2" $poolValue + $cfg | ConvertTo-Json -Depth 10 | Set-Content -Path $Dst -Encoding UTF8 + Write-Host "Wrote runner config to $Dst (Server=tcp:$SqlServer,1433; Initial Catalog=$DbName)" +} + +Write-RunnerConfig -Dst $RunnerConfig + +# -SwitchUnderTest needs two DIFFERENT runner configs (baseline runs the switch off, current runs it +# on), so stamp out two more copies here alongside the shared one above. Everything else in them is +# identical, so any measured delta is attributable to the switch alone. +$BaselineRunnerConfig = "" +$CurrentRunnerConfig = "" +if (-not [string]::IsNullOrEmpty($SwitchUnderTest)) { + $BaselineRunnerConfig = Join-Path $RepoRoot "perf-runnerconfig-baseline.json" + $CurrentRunnerConfig = Join-Path $RepoRoot "perf-runnerconfig-current.json" + Write-RunnerConfig -Dst $BaselineRunnerConfig -SwitchName $SwitchUnderTest -SwitchValue "false" + Write-RunnerConfig -Dst $CurrentRunnerConfig -SwitchName $SwitchUnderTest -SwitchValue "true" +} #################################################################################################### # 4 & 5. Run the benchmarks, pinned to the reserved client CPU set. @@ -685,6 +742,11 @@ if (-not [string]::IsNullOrEmpty($BaselineVersion)) { $baselineSource = Initialize-BaselineSource -Ref $BaselineSourceRef $BaselineLabel = $baselineSource.Label $BaselineProject = $baselineSource.Project +} elseif (-not [string]::IsNullOrEmpty($SwitchUnderTest)) { + # Switch A/B: SAME source/project for both passes ($BaselineProject/$BaselineBuildArgs are + # already the candidate's, set above), so only the runner config differs (see + # $BaselineRunnerConfig/$CurrentRunnerConfig written above: the named switch off vs on). + $BaselineLabel = "$SwitchUnderTest=false" } # Record the resolved baseline label (for a source baseline this is '@') in the results @@ -701,8 +763,17 @@ if ((-not [string]::IsNullOrEmpty($BaselineLabel)) -and ($RunMode -eq "interleav # orchestrator run one unit at a time (baseline then candidate) and confirm any flagged # regression across N passes before it counts toward the gate. #################################################################################################### - $baselineExeDir = Build-Variant "baseline" $BaselineProject $BaselineBuildArgs - $currentExeDir = Build-Variant "current" $PerfProject @() + if (-not [string]::IsNullOrEmpty($SwitchUnderTest)) { + # Switch A/B measures one build against itself with a switch flipped, so building the same + # project twice would just burn several minutes producing identical bits. Build once and + # point both variants at it; the orchestrator runs each variant in its own working directory + # (rep//), so a shared exe dir cannot cross-contaminate their artifacts. + $currentExeDir = Build-Variant "current" $PerfProject @() + $baselineExeDir = $currentExeDir + } else { + $baselineExeDir = Build-Variant "baseline" $BaselineProject $BaselineBuildArgs + $currentExeDir = Build-Variant "current" $PerfProject @() + } $interleaveArgs = @( "--baseline-exe-dir", $baselineExeDir, @@ -714,6 +785,12 @@ if ((-not [string]::IsNullOrEmpty($BaselineLabel)) -and ($RunMode -eq "interleav "--baseline-version", $BaselineLabel, "--client-cpus", "$($env:PERF_CLIENT_CPUS)" ) + # -SwitchUnderTest: baseline and current subprocesses need DIFFERENT RUNNER_CONFIG values (the + # switch off vs on), even though both are otherwise the same build/env; every other baseline + # flavour keeps sharing the single ambient RUNNER_CONFIG set above. + if (-not [string]::IsNullOrEmpty($SwitchUnderTest)) { + $interleaveArgs += @("--baseline-runner-config", $BaselineRunnerConfig, "--current-runner-config", $CurrentRunnerConfig) + } if ($FailOnRegression) { Write-Host "Regression gate ENABLED: a CONFIRMED candidate-slower regression (> $RegressionThreshold%) will fail the run." $interleaveArgs += "--fail-on-regression" @@ -723,7 +800,11 @@ if ((-not [string]::IsNullOrEmpty($BaselineLabel)) -and ($RunMode -eq "interleav } elseif (-not [string]::IsNullOrEmpty($BaselineLabel)) { # --- Legacy sequential path: full baseline pass, then full candidate pass, then compare ------- + # -SwitchUnderTest needs a different RUNNER_CONFIG per pass; every other baseline flavour + # keeps using the single ambient RUNNER_CONFIG set above (unchanged behaviour). + if (-not [string]::IsNullOrEmpty($SwitchUnderTest)) { $env:RUNNER_CONFIG = $BaselineRunnerConfig } Invoke-PerfPass "baseline" $BaselineProject $BaselineBuildArgs + if (-not [string]::IsNullOrEmpty($SwitchUnderTest)) { $env:RUNNER_CONFIG = $CurrentRunnerConfig } Invoke-PerfPass "current" $PerfProject @() Write-Host "Comparing current branch against baseline $BaselineLabel ..." diff --git a/eng/pipelines/perf/scripts/run-perf-tests.sh b/eng/pipelines/perf/scripts/run-perf-tests.sh index 4df2eec8e3..70ed796b4a 100755 --- a/eng/pipelines/perf/scripts/run-perf-tests.sh +++ b/eng/pipelines/perf/scripts/run-perf-tests.sh @@ -62,10 +62,22 @@ confirmationRuns="3" useManagedSniOnWindows="" useOptimizedAsyncBehaviour="" useConnectionPoolV2="" +# Alternative to --baseline-version/--baseline-source-ref: an A/B experiment on ONE runner-config +# switch. Both passes build the SAME source; only the named switch differs (baseline=false, +# current=true), which is the only way to compare a switch whose value is latched process-wide (e.g. +# UseConnectionPoolV2 is read and cached the first time a pool is created). Mutually exclusive with +# the other two baseline selectors, and overrides the matching --use-* flag (which would otherwise be +# ambiguous: one value cannot describe two passes). +switchUnderTest="" +# Runner-config switches this script is allowed to A/B. Restricted to a known list so a typo fails +# fast here instead of silently writing an inert key into the runner config and reporting a +# meaningless zero-delta comparison. +SUPPORTED_SWITCHES=("UseConnectionPoolV2" "UseOptimizedAsyncBehaviour" "UseManagedSniOnWindows") usage() { echo "Usage: $0 [--configuration ] [--framework ] [--results-subdir ]" \ - "[--baseline-version | --baseline-source-ref [--baseline-repo-url ]]" \ + "[--baseline-version | --baseline-source-ref [--baseline-repo-url ] |" \ + "--switch-under-test <${SUPPORTED_SWITCHES[*]}>]" \ "[--regression-threshold ] [--fail-on-regression]" \ "[--run-mode interleaved|sequential] [--confirmation-runs ]" \ "[--use-managed-sni-on-windows true|false] [--use-optimized-async-behaviour true|false]" \ @@ -80,6 +92,7 @@ while [[ $# -gt 0 ]]; do --baseline-version) baselineVersion="$2"; shift 2 ;; --baseline-source-ref) baselineSourceRef="$2"; shift 2 ;; --baseline-repo-url) baselineRepoUrl="$2"; shift 2 ;; + --switch-under-test) switchUnderTest="$2"; shift 2 ;; --regression-threshold) regressionThreshold="$2"; shift 2 ;; --fail-on-regression) failOnRegression="true"; shift 1 ;; --run-mode) runMode="$2"; shift 2 ;; @@ -100,12 +113,26 @@ case "${runMode}" in *) echo "ERROR: --run-mode must be 'interleaved' or 'sequential' (got '${runMode}')." >&2 usage; exit 2 ;; esac -# 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. +# The three baseline selectors describe different builds/configs of the same "baseline" pass, so +# requesting more than one is always a mistake; fail fast rather than silently honouring one of them. if [[ -n "${baselineVersion}" && -n "${baselineSourceRef}" ]]; then echo "ERROR: --baseline-version and --baseline-source-ref are mutually exclusive." >&2 usage; exit 2 fi +if [[ -n "${switchUnderTest}" && ( -n "${baselineVersion}" || -n "${baselineSourceRef}" ) ]]; then + echo "ERROR: --switch-under-test is mutually exclusive with --baseline-version and --baseline-source-ref: it compares the SAME source build with one switch flipped, so mixing in a source change would make the delta unattributable." >&2 + usage; exit 2 +fi +if [[ -n "${switchUnderTest}" ]]; then + switchSupported="false" + for supported in "${SUPPORTED_SWITCHES[@]}"; do + [[ "${switchUnderTest}" == "${supported}" ]] && switchSupported="true" + done + if [[ "${switchSupported}" != "true" ]]; then + echo "ERROR: --switch-under-test must be one of: ${SUPPORTED_SWITCHES[*]} (got '${switchUnderTest}')." >&2 + usage; exit 2 + fi +fi if ! [[ "${confirmationRuns}" =~ ^[0-9]+$ ]] || [[ "${confirmationRuns}" -lt 1 ]]; then echo "ERROR: --confirmation-runs must be a positive integer (got '${confirmationRuns}')." >&2 usage; exit 2 @@ -120,6 +147,18 @@ validate_bool() { # $1 = flag name (for the message), $2 = value validate_bool use-managed-sni-on-windows "${useManagedSniOnWindows}" validate_bool use-optimized-async-behaviour "${useOptimizedAsyncBehaviour}" validate_bool use-connection-pool-v2 "${useConnectionPoolV2}" +# --switch-under-test 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. +case "${switchUnderTest}" in + UseConnectionPoolV2) conflictingValue="${useConnectionPoolV2}"; conflictingFlag="--use-connection-pool-v2" ;; + UseOptimizedAsyncBehaviour) conflictingValue="${useOptimizedAsyncBehaviour}"; conflictingFlag="--use-optimized-async-behaviour" ;; + UseManagedSniOnWindows) conflictingValue="${useManagedSniOnWindows}"; conflictingFlag="--use-managed-sni-on-windows" ;; + *) conflictingValue=""; conflictingFlag="" ;; +esac +if [[ -n "${conflictingValue}" ]]; then + echo "WARNING: ${conflictingFlag} is ignored when --switch-under-test is ${switchUnderTest} (baseline forces false, current forces true)." >&2 +fi #################################################################################################### # Resolve paths @@ -145,6 +184,7 @@ echo " Framework : ${framework}" echo " Results dir : ${RESULTS_DIR}" echo " Baseline ver : ${baselineVersion:-}" echo " Baseline ref : ${baselineSourceRef:-}" +echo " Switch A/B : ${switchUnderTest:-}${switchUnderTest:+ (baseline=false vs current=true)}" echo " Run mode : ${runMode} (confirmation runs: ${confirmationRuns})" echo " SQL_SERVER : ${SQL_SERVER:-}" echo " PERF_CLIENT_CPUS: ${PERF_CLIENT_CPUS:-}" @@ -355,10 +395,20 @@ export PERF_CFG_USE_MANAGED_SNI="${useManagedSniOnWindows}" export PERF_CFG_USE_OPTIMIZED_ASYNC="${useOptimizedAsyncBehaviour}" export PERF_CFG_USE_CONNECTION_POOL_V2="${useConnectionPoolV2}" -python3 - "$PERF_DIR/runnerconfig.jsonc" "$RUNNER_CONFIG" <<'PY' +# write_runner_config [switch_name] [switch_value] +# Writes one runner config (checked-in runnerconfig.jsonc + injected connection string + behaviour +# overrides) to . When is given, that config key is forced to +# ("true"/"false") regardless of the corresponding PERF_CFG_* value -- used by --switch-under-test, +# which needs a different value for the same switch in each pass. With no switch name the config is +# built purely from the PERF_CFG_* values, exactly as before. +write_runner_config() { + local dst="$1" + local switch_name="${2:-}" + local switch_value="${3:-}" + python3 - "$PERF_DIR/runnerconfig.jsonc" "$dst" "$switch_name" "$switch_value" <<'PY' import json, os, re, sys -src, dst = sys.argv[1], sys.argv[2] +src, dst, switch_name, switch_value = sys.argv[1], sys.argv[2], sys.argv[3], sys.argv[4] with open(src, "r", encoding="utf-8-sig") as fh: text = fh.read() @@ -384,13 +434,16 @@ cfg["ConnectionString"] = ( # Apply the optional SqlClient behaviour overrides supplied by the pipeline. An empty value leaves # the checked-in default untouched; otherwise the flag is forced to the requested boolean so the -# benchmarks run with (and PerfRun.Config records) exactly the requested behaviour. +# benchmarks run with (and PerfRun.Config records) exactly the requested behaviour. The +# switch-under-test override (when this config names one) takes precedence over the corresponding +# PERF_CFG_* value, so a single checked-in template can be stamped out per pass with that one switch +# flipped and everything else identical. for env_name, cfg_key in ( ("PERF_CFG_USE_MANAGED_SNI", "UseManagedSniOnWindows"), ("PERF_CFG_USE_OPTIMIZED_ASYNC", "UseOptimizedAsyncBehaviour"), ("PERF_CFG_USE_CONNECTION_POOL_V2", "UseConnectionPoolV2"), ): - val = os.environ.get(env_name, "") + val = switch_value if cfg_key == switch_name else os.environ.get(env_name, "") if val != "": cfg[cfg_key] = (val.lower() == "true") @@ -399,6 +452,21 @@ with open(dst, "w", encoding="utf-8") as fh: print(f"Wrote runner config to {dst} (Server=tcp:{server},1433; Initial Catalog={db})") PY +} + +write_runner_config "${RUNNER_CONFIG}" + +# --switch-under-test needs two DIFFERENT runner configs (baseline runs the switch off, current runs +# it on), so stamp out two more copies here alongside the shared one above. Everything else in them +# is identical, so any measured delta is attributable to the switch alone. +BASELINE_RUNNER_CONFIG="" +CURRENT_RUNNER_CONFIG="" +if [[ -n "${switchUnderTest}" ]]; then + BASELINE_RUNNER_CONFIG="${REPO_ROOT}/perf-runnerconfig-baseline.json" + CURRENT_RUNNER_CONFIG="${REPO_ROOT}/perf-runnerconfig-current.json" + write_runner_config "${BASELINE_RUNNER_CONFIG}" "${switchUnderTest}" "false" + write_runner_config "${CURRENT_RUNNER_CONFIG}" "${switchUnderTest}" "true" +fi #################################################################################################### # 4 & 5. Run the benchmarks, pinned to the reserved client CPU set. @@ -683,6 +751,11 @@ elif [[ -n "${baselineSourceRef}" ]]; then prepare_baseline_source "${baselineSourceRef}" baselineLabel="${BASELINE_SRC_LABEL}" baselineProject="${BASELINE_PERF_PROJECT}" +elif [[ -n "${switchUnderTest}" ]]; then + # Switch A/B: SAME source/project for both passes (baselineProject/baselineBuildArgs are already + # the candidate's, set above), so only the runner config differs (see BASELINE_RUNNER_CONFIG / + # CURRENT_RUNNER_CONFIG written above: the named switch off vs on). + baselineLabel="${switchUnderTest}=false" fi # Record the resolved baseline label (for a source baseline this is '@') in the results @@ -699,12 +772,24 @@ if [[ -n "${baselineLabel}" && "${runMode}" == "interleaved" ]]; then # orchestrator run one unit at a time (baseline then candidate) and confirm any flagged # regression across N passes before it counts toward the gate. ################################################################################################ - build_variant "baseline" "${baselineProject}" ${baselineBuildArgs[@]+"${baselineBuildArgs[@]}"} - build_variant "current" "${PERF_PROJECT}" + if [[ -n "${switchUnderTest}" ]]; then + # Switch A/B measures one build against itself with a switch flipped, so building the same + # project twice would just burn several minutes producing identical bits. Build once and + # point both variants at it; the orchestrator runs each variant in its own working directory + # (rep//), so a shared exe dir cannot cross-contaminate their artifacts. + build_variant "current" "${PERF_PROJECT}" + baselineExeDir="${REPO_ROOT}/perf-build-current" + currentExeDir="${REPO_ROOT}/perf-build-current" + else + build_variant "baseline" "${baselineProject}" ${baselineBuildArgs[@]+"${baselineBuildArgs[@]}"} + build_variant "current" "${PERF_PROJECT}" + baselineExeDir="${REPO_ROOT}/perf-build-baseline" + currentExeDir="${REPO_ROOT}/perf-build-current" + fi interleave_args=( - --baseline-exe-dir "${REPO_ROOT}/perf-build-baseline" - --current-exe-dir "${REPO_ROOT}/perf-build-current" + --baseline-exe-dir "${baselineExeDir}" + --current-exe-dir "${currentExeDir}" --assembly "PerformanceTests.dll" --results-dir "${RESULTS_DIR}" --threshold "${regressionThreshold}" @@ -712,6 +797,15 @@ if [[ -n "${baselineLabel}" && "${runMode}" == "interleaved" ]]; then --baseline-version "${baselineLabel}" --client-cpus "${PERF_CLIENT_CPUS:-}" ) + # --switch-under-test: baseline and current subprocesses need DIFFERENT RUNNER_CONFIG values (the + # switch off vs on), even though both are otherwise the same build/env; every other baseline + # flavour keeps sharing the single ambient RUNNER_CONFIG set above. + if [[ -n "${switchUnderTest}" ]]; then + interleave_args+=( + --baseline-runner-config "${BASELINE_RUNNER_CONFIG}" + --current-runner-config "${CURRENT_RUNNER_CONFIG}" + ) + fi if [[ "${failOnRegression}" == "true" ]]; then echo "Regression gate ENABLED: a CONFIRMED candidate-slower regression (> ${regressionThreshold}%) will fail the run." interleave_args+=(--fail-on-regression) @@ -721,7 +815,15 @@ if [[ -n "${baselineLabel}" && "${runMode}" == "interleaved" ]]; then elif [[ -n "${baselineLabel}" ]]; then # --- Legacy sequential path: full baseline pass, then full candidate pass, then compare ------- + # --switch-under-test needs a different RUNNER_CONFIG per pass; every other baseline flavour + # keeps using the single ambient RUNNER_CONFIG exported above (unchanged behaviour). + if [[ -n "${switchUnderTest}" ]]; then + export RUNNER_CONFIG="${BASELINE_RUNNER_CONFIG}" + fi run_pass "baseline" "${baselineProject}" ${baselineBuildArgs[@]+"${baselineBuildArgs[@]}"} + if [[ -n "${switchUnderTest}" ]]; then + export RUNNER_CONFIG="${CURRENT_RUNNER_CONFIG}" + fi run_pass "current" "${PERF_PROJECT}" echo "Comparing current branch against baseline ${baselineLabel} ..." diff --git a/eng/pipelines/perf/sqlclient-perf-switch-pipeline.yml b/eng/pipelines/perf/sqlclient-perf-switch-pipeline.yml new file mode 100644 index 0000000000..0405f0f7bd --- /dev/null +++ b/eng/pipelines/perf/sqlclient-perf-switch-pipeline.yml @@ -0,0 +1,235 @@ +#################################################################################################### +# Licensed to the .NET Foundation under one or more agreements. The .NET Foundation licenses this +# file to you under the MIT license. See the LICENSE file in the project root for more information. +#################################################################################################### + +# SqlClient Switch Experiment Performance pipeline. +# +# Third sibling of sqlclient-perf-pipeline.yml and sqlclient-perf-pr-pipeline.yml. It runs the SAME +# benchmarks, on the SAME Perf Test Lab extends template (v1/Perf.Test.Job.yml@PerfTemplates), +# through the SAME on-VM scripts. What differs is the QUESTION it answers: +# +# * perf pipeline - "has this branch regressed against a released package?" (source varies) +# * perf-pr pipeline - "has this PR regressed the branch it merges into?" (source varies) +# * THIS pipeline - "what does this AppContext/runner switch cost or buy?" (CONFIG varies) +# +# Both passes here build the SAME source (whatever branch the run is queued on); only ONE runner +# config switch differs between them - baseline runs it OFF, current runs it ON. Two separate +# processes are required because these switches are latched process-wide (e.g. UseConnectionPoolV2 is +# read and cached the first time a connection pool is created), so they cannot be toggled between +# benchmarks inside a single run. Queue this pipeline on a PR branch to ask "what does the switch do +# to my change?", or on main to ask "what does the switch do to main?". +# +# * No Kusto - switch-experiment results are NEVER ingested into the perf database, and this is why +# the mode lives in its own pipeline rather than as a flag on the other two: +# - both PerfRun rows would share one DerivedRunId (driver|commit|pipelineRunId), +# because both passes are the same commit in the same pipeline run; +# - PerfRun.Config is stamped once per run, so the two rows could not record the +# differing switch values that are the entire point of the experiment; +# - nothing marks a row as an experiment, so the switch-ON pass would be +# indistinguishable from an ordinary measurement of the branch and would distort +# the very trends the other two pipelines exist to protect. +# Having no ADX variable group and no translate/ingest steps in this file makes that +# exclusion structural rather than a conditional someone can flip by accident. +# The comparison report and the raw BenchmarkDotNet artifacts are still published. +# +# Everything else - platform/VM provisioning, benchmark run model, noise controls - is identical to +# the other two pipelines; see eng/pipelines/perf/README.md. + +# Set the pipeline run name to the day-of-year and the daily run counter. +name: $(DayOfYear)$(Rev:rr) + +# Manual/queue-time only. A full perf run occupies a dedicated host for hours, and a switch +# experiment is an investigation someone opts into deliberately, never an automatic gate. +pr: none +trigger: none + +parameters: + + # The single runner-config switch under test. Baseline runs it false, current runs it true, so the + # reported delta is "what turning this switch ON does". Restricted to the switches the run scripts + # know how to stamp into the runner config; the scripts re-validate and fail fast on anything else. + - name: switchUnderTest + displayName: Switch under test (baseline=off vs current=on) + type: string + default: UseConnectionPoolV2 + values: + - UseConnectionPoolV2 + - UseOptimizedAsyncBehaviour + - UseManagedSniOnWindows + + # Target OS for the perf VM and the benchmark client. + - name: platform + displayName: Platform + type: string + default: linux + values: + - linux + - windows + + # The .NET runtime the benchmarks are executed against. Must be one of the target frameworks of + # the PerformanceTests project (net8.0/net9.0/net10.0). + - name: dotnetFramework + displayName: .NET Framework (TFM) + type: string + default: net9.0 + values: + - net8.0 + - net9.0 + - net10.0 + + # Maximum time (minutes) the template waits for the benchmark run on the VM before timing out. + # Matches the nightly pipeline rather than the PR pipeline: both passes here share ONE build (the + # source is identical on both sides), so there is no second driver build to pay for. + - name: testTimeoutMinutes + displayName: Test Timeout (minutes) + type: number + default: 180 + + # Percent difference (switch-on vs switch-off mean) the comparison flags. + - name: regressionThreshold + displayName: Difference Threshold (%) + type: number + default: 10 + + # Whether a confirmed "switch ON is slower than switch OFF" result should FAIL the run. + # + # NOTE: this maps onto the run scripts' --fail-on-regression gate, but it does NOT mean the same + # thing as it does in the other two pipelines. There, a regression means the branch got worse and + # failing is a quality gate. Here it is a RESULT: the switch made things slower. That is often + # exactly what you queued the run to find out, so this defaults to false and should only be enabled + # when you are asserting "this switch must not be a slowdown" (e.g. before flipping its default). + - name: failIfSwitchSlower + displayName: Fail run if the switch is slower + type: boolean + default: false + + # Benchmark run model (wiki 339 §2.2/§2.3/§2.6): + # interleaved -> run one benchmark unit at a time, switch-off and switch-on back-to-back, and + # confirm any flagged difference across N passes (best-of-N). Noise-resistant + # default, and especially valuable here because the two passes are otherwise + # identical, so any systematic drift between them is pure measurement noise. + # sequential -> legacy: run the whole switch-off suite, then the whole switch-on suite, compare. + - name: benchmarkRunMode + displayName: Benchmark run mode + type: string + default: interleaved + values: + - interleaved + - sequential + + # Best-of-N: total interleaved passes for a flagged unit before a difference is confirmed + # (1 disables confirmation). Only used when benchmarkRunMode = interleaved. + - name: confirmationRuns + displayName: Confirmation runs (best-of-N) + type: number + default: 3 + +# Fixed (non-configurable) constants for this pipeline. These are intentionally NOT parameters or +# library variables: they are invariant for the SqlClient perf pipelines. +# * buildConfiguration = Release - perf numbers are only meaningful in Release. +# * sourcesSubDir = dotnet-sqlclient - folder 'self' checks out into under the template's +# MULTI-REPO checkout ($(Build.SourcesDirectory)/); +# must match the ADO repository name. +# +# The other pipelines additionally expose UseManagedSniOnWindows / UseOptimizedAsyncBehaviour / +# UseConnectionPoolV2 as queue-time flags applied to BOTH passes. This pipeline deliberately does +# not: an experiment that varies one switch while others are also moved off their checked-in defaults +# produces a delta nobody can attribute. Every switch except the one under test is therefore left at +# its runnerconfig.jsonc default (which is what those parameters' defaults already are). +# +# NOTE: like sqlclient-perf-pr-pipeline.yml, this pipeline does NOT reference the 'ADX Cluster +# Variables' group and has no translate/ingest steps. See the header comment for why that is +# structural here rather than conditional. +variables: + + # Pre-computed testScriptArgs chunks. The Windows entry point is PowerShell and binds + # '-PascalCase' params, while the Linux one is bash and parses '--kebab-case' flags, so the + # argument STYLE differs by platform. Assembling the final args from these per-platform chunks + # keeps the single testScriptArgs below readable. No baseline-selector args appear here at all: + # --switch-under-test IS the baseline selector for this pipeline, and the run scripts reject it + # being combined with --baseline-version / --baseline-source-ref. + - ${{ if eq(parameters.platform, 'windows') }}: + - name: PerfArgsCommon + value: '-Configuration Release -Framework ${{ parameters.dotnetFramework }} -ResultsSubdir perf-results -RegressionThreshold ${{ parameters.regressionThreshold }} -RunMode ${{ parameters.benchmarkRunMode }} -ConfirmationRuns ${{ parameters.confirmationRuns }} -SwitchUnderTest ${{ parameters.switchUnderTest }}' + - ${{ else }}: + - name: PerfArgsCommon + value: '--configuration Release --framework ${{ parameters.dotnetFramework }} --results-subdir perf-results --regression-threshold ${{ parameters.regressionThreshold }} --run-mode ${{ parameters.benchmarkRunMode }} --confirmation-runs ${{ parameters.confirmationRuns }} --switch-under-test ${{ parameters.switchUnderTest }}' + + # Gate flag, only when the run is asserting the switch must not be a slowdown. + - ${{ if and(eq(parameters.platform, 'windows'), eq(parameters.failIfSwitchSlower, true)) }}: + - name: PerfArgsFail + value: '-FailOnRegression' + - ${{ elseif eq(parameters.failIfSwitchSlower, true) }}: + - name: PerfArgsFail + value: '--fail-on-regression' + - ${{ else }}: + - name: PerfArgsFail + value: '' + +# Reference the PerfTest repository that hosts the reusable extends template. Driver-team projects +# have been onboarded with the Agent Pools and Service Connections required to consume it. +resources: + repositories: + - repository: PerfTemplates + type: git + name: InternalDriverTools/PerfTest + ref: refs/heads/main + +# Consume the Perf Test Lab template. The template defines the whole job/stage structure; we only +# pass parameters into it. +extends: + template: v1/Perf.Test.Job.yml@PerfTemplates + parameters: + platform: ${{ parameters.platform }} + + # The entire driver source tree is copied to the VM so the benchmarks (which reference + # Microsoft.Data.SqlClient as a project) can be built from source against the current commit. + # Under the template's multi-repo checkout, 'self' lands in a repo-named subfolder. + testRootDir: $(Build.SourcesDirectory)/dotnet-sqlclient + + # Entry-point script (relative to testRootDir), selected by platform. Linux uses bash, Windows + # uses PowerShell, per the template's .sh/.ps1 convention. Same scripts as the other two perf + # pipelines: the switch experiment is just a different baseline selector. + ${{ if eq(parameters.platform, 'windows') }}: + testScript: eng/pipelines/perf/scripts/run-perf-tests.ps1 + ${{ else }}: + testScript: eng/pipelines/perf/scripts/run-perf-tests.sh + + # Arguments forwarded to the script. The script also reads SQL_SERVER, SQL_PASSWORD and + # PERF_CLIENT_CPUS directly from the VM session environment (injected by the template). Empty + # chunks collapse to harmless extra spaces that both PowerShell and bash arg parsing ignore. + testScriptArgs: '$(PerfArgsCommon) $(PerfArgsFail)' + + # Subfolder (relative to testRootDir on the VM) the script writes results into. The template + # copies this VM folder back to the agent, but always lands it at a fixed location: + # $(Build.ArtifactStagingDirectory)/results (and publishes it as the 'perf-results' artifact). + testResultsSubDir: perf-results + + testTimeoutMinutes: ${{ parameters.testTimeoutMinutes }} + jobName: SqlClientPerfSwitch + + # Post-test steps run on the agent after results have been copied back. The template already + # publishes the results artifact and attaches any top-level results/*.md as run summaries; this + # step additionally surfaces the BenchmarkDotNet markdown reports in the build log. There is no + # Kusto translation/ingestion here by design - see the header comment. + steps: + # Tag the build with the switch that was measured, so switch experiments are identifiable at a + # glance in the ADO build list and are never mistaken for an ordinary baseline comparison. + # Unlike the PR pipeline there is nothing to read back from the VM: the switch name is fixed at + # queue time, and both passes are the commit the run was queued on. + # NOTE: the tag intentionally has no ':' - build tags are placed in the request URL path and a + # colon trips ADO's "potentially dangerous Request.Path" filter. + - bash: | + echo "##vso[build.addbuildtag]Switch ${{ parameters.switchUnderTest }}" + displayName: 'Tag build with switch under test' + condition: succeededOrFailed() + + - task: Bash@3 + displayName: 'Show performance results' + condition: succeededOrFailed() + inputs: + targetType: filePath + filePath: $(Build.SourcesDirectory)/dotnet-sqlclient/eng/pipelines/perf/scripts/show_perf_results.sh + env: + RESULTS_DIR: $(Build.ArtifactStagingDirectory)/results From 7a6c998ec17d4cd4fd819a4d51c96a15d74aa6ee Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Thu, 13 Aug 2026 14:30:38 -0700 Subject: [PATCH 02/34] Rename switch pipeline to sqlclient-perf-experiment-pipeline.yml Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- eng/pipelines/perf/README.md | 16 ++++++++-------- ...ml => sqlclient-perf-experiment-pipeline.yml} | 2 +- 2 files changed, 9 insertions(+), 9 deletions(-) rename eng/pipelines/perf/{sqlclient-perf-switch-pipeline.yml => sqlclient-perf-experiment-pipeline.yml} (99%) diff --git a/eng/pipelines/perf/README.md b/eng/pipelines/perf/README.md index 0243a84f99..4ae849d1e3 100644 --- a/eng/pipelines/perf/README.md +++ b/eng/pipelines/perf/README.md @@ -12,7 +12,7 @@ database. | ---- | ------- | | `sqlclient-perf-pipeline.yml` | The main (manual/nightly) pipeline. Extends `v1/Perf.Test.Job.yml@PerfTemplates`. Baseline = released NuGet package; ingests into Kusto. | | `sqlclient-perf-pr-pipeline.yml` | PR pipeline. Same template, same scripts, same options; baseline = **`main` branch source**; **no Kusto ingestion**. | -| `sqlclient-perf-switch-pipeline.yml` | Switch experiment pipeline. Same template, same scripts; both passes build the **same source** and differ only in one runner-config switch; **no Kusto ingestion**. | +| `sqlclient-perf-experiment-pipeline.yml` | Experiment pipeline. Same template, same scripts; both passes build the **same source** and differ only in one runner-config switch; **no Kusto ingestion**. | | `scripts/run-perf-tests.sh` | Linux on-VM entry point: install SDK, create DB, run benchmarks (interleaved or sequential), compare. Baseline is a released package (`--baseline-version`), another git ref's source (`--baseline-source-ref`), or the same source with one runner-config switch flipped off (`--switch-under-test`). | | `scripts/run-perf-tests.ps1` | Windows equivalent (ProcessorAffinity instead of `taskset`). | | `scripts/interleave_perf.py` | Interleaved + best-of-N orchestrator: runs each unit baseline↔candidate back-to-back and confirms regressions across N passes. | @@ -152,7 +152,7 @@ the comparison (and one removed by the PR as `removed`) instead of failing the r share the single generated runner config (`RUNNER_CONFIG` / `DATATYPES_CONFIG` env vars), so connection string and behaviour flags are identical on both sides. -## Switch experiment pipeline (`sqlclient-perf-switch-pipeline.yml`) +## Experiment pipeline (`sqlclient-perf-experiment-pipeline.yml`) The three perf pipelines are the same benchmarks, template and scripts pointed at three different questions. Two of them vary the **source** under measurement; the third varies the **config**: @@ -161,9 +161,9 @@ questions. Two of them vary the **source** under measurement; the third varies t | --- | --- | --- | --- | | Has this branch regressed against a released package? | `sqlclient-perf-pipeline.yml` | released NuGet package | queued branch | | Does my PR regress the branch it merges into? | `sqlclient-perf-pr-pipeline.yml` | `main` source | queued branch | -| What does this switch cost or buy? | `sqlclient-perf-switch-pipeline.yml` | queued branch, switch **off** | queued branch, switch **on** | +| What does this switch cost or buy? | `sqlclient-perf-experiment-pipeline.yml` | queued branch, switch **off** | queued branch, switch **on** | -`sqlclient-perf-switch-pipeline.yml` picks one runner-config switch via the `switchUnderTest` +`sqlclient-perf-experiment-pipeline.yml` picks one runner-config switch via the `switchUnderTest` queue-time parameter (`UseConnectionPoolV2`, `UseOptimizedAsyncBehaviour` or `UseManagedSniOnWindows`) and runs the baseline pass with it `false` and the current pass with it `true`. Both passes measure the **same commit** — the branch the run is queued on — so queue it on a @@ -225,7 +225,7 @@ The `PerformanceTests` project references Microsoft.Data.SqlClient two ways, sel that ref's driver source. Used by the PR pipeline. - **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 switch pipeline. + differing only in the runner config each pass is handed. Used by the experiment pipeline. The VM's `NuGet.config` exposes only the governed feed, and CPM rejects multiple unmapped sources (`NU1507`). The baseline pass therefore restores through a **dedicated single-source config** @@ -387,9 +387,9 @@ translated NDJSON as the `perf-kusto-payloads` artifact for manual/backfill inge 3. After the run, review the **run summary** (comparison, labelled `@`) and the `perf-results` artifact. The build is tagged **`Baseline `**. -### Running the switch pipeline +### Running the experiment pipeline -1. Open the **switch** performance test pipeline (`sqlclient-perf-switch-pipeline.yml`) in Azure +1. Open the **experiment** performance test pipeline (`sqlclient-perf-experiment-pipeline.yml`) in Azure DevOps and select **Run pipeline**. 2. Choose the branch whose behaviour you want to measure — `main` to characterise the switch on its own, or a PR branch to characterise it against that change — and pick `switchUnderTest`. There is @@ -407,7 +407,7 @@ translated NDJSON as the `perf-kusto-payloads` artifact for manual/backfill inge | Baseline restore fails to find MDS | `baselineVersion` isn't a published NuGet.org version, or the VM has no outbound access to `api.nuget.org`. | | Baseline pass fails to compile: `CS1061 ... does not contain a definition for ` | A benchmark calls an MDS API newer than `baselineVersion`. Guard it with the `MDS_GE_` constants and add an older fallback — see [Benchmarks must compile against the oldest baseline](#benchmarks-must-compile-against-the-oldest-baseline). | | No comparison / summary | The baseline pass was skipped (empty `baselineVersion` / `baselineSourceRef`) or one pass produced no `*-report-full.json`. | -| `--switch-under-test is mutually exclusive with --baseline-version and --baseline-source-ref` | A switch experiment was combined with a source baseline. The switch pipeline never does this; if you are invoking the scripts directly, clear the baseline selector — varying source and config at once makes the delta unattributable. | +| `--switch-under-test is mutually exclusive with --baseline-version and --baseline-source-ref` | A switch experiment was combined with a source baseline. The experiment pipeline never does this; if you are invoking the scripts directly, clear the baseline selector — varying source and config at once makes the delta unattributable. | | Switch experiment shows a ~0% delta everywhere | Expected for benchmarks the switch does not touch. If *every* benchmark is flat, check the run log's `Switch A/B` line actually names the switch, and that the switch is one the driver reads at startup via the runner config. | | Baseline source ref not found (PR pipeline) | `baselineSourceRef` is not a branch on `origin`, and the fallback `git clone --branch ` of `baselineRepoUrl` also failed (ref does not exist there, or the VM has no outbound access to the remote). The reason git gave is echoed into the build log and saved to `/diagnostics/git-*.log`. | | `Fetching baseline ref ... from the checkout's origin` is the last line, then the job stalls | Should no longer happen. The checkout is copied to the VM without credentials (ADO's checkout task defaults to `persistCredentials: false`), so a fetch from an authenticated `origin` used to sit on a `Username for ...` prompt forever. All network git calls now run with `GIT_TERMINAL_PROMPT=0`, no credential helper, stdin closed, and a `GIT_NET_TIMEOUT_SECS` (default 300s) hard timeout, so this fails in under a second and falls back to cloning `baselineRepoUrl`. A fetch failure here is expected and harmless on ADO. | diff --git a/eng/pipelines/perf/sqlclient-perf-switch-pipeline.yml b/eng/pipelines/perf/sqlclient-perf-experiment-pipeline.yml similarity index 99% rename from eng/pipelines/perf/sqlclient-perf-switch-pipeline.yml rename to eng/pipelines/perf/sqlclient-perf-experiment-pipeline.yml index 0405f0f7bd..7539c97bfa 100644 --- a/eng/pipelines/perf/sqlclient-perf-switch-pipeline.yml +++ b/eng/pipelines/perf/sqlclient-perf-experiment-pipeline.yml @@ -207,7 +207,7 @@ extends: testResultsSubDir: perf-results testTimeoutMinutes: ${{ parameters.testTimeoutMinutes }} - jobName: SqlClientPerfSwitch + jobName: SqlClientPerfExperiment # Post-test steps run on the agent after results have been copied back. The template already # publishes the results artifact and attaches any top-level results/*.md as run summaries; this From 14fb9bd3ca0f2388319a6dab4474b686afebbac4 Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Thu, 13 Aug 2026 14:30:58 -0700 Subject: [PATCH 03/34] Drop -pipeline suffix from perf experiment pipeline filename Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- eng/pipelines/perf/README.md | 10 +++++----- ...ment-pipeline.yml => sqlclient-perf-experiment.yml} | 0 2 files changed, 5 insertions(+), 5 deletions(-) rename eng/pipelines/perf/{sqlclient-perf-experiment-pipeline.yml => sqlclient-perf-experiment.yml} (100%) diff --git a/eng/pipelines/perf/README.md b/eng/pipelines/perf/README.md index 4ae849d1e3..731d75cf5d 100644 --- a/eng/pipelines/perf/README.md +++ b/eng/pipelines/perf/README.md @@ -12,7 +12,7 @@ database. | ---- | ------- | | `sqlclient-perf-pipeline.yml` | The main (manual/nightly) pipeline. Extends `v1/Perf.Test.Job.yml@PerfTemplates`. Baseline = released NuGet package; ingests into Kusto. | | `sqlclient-perf-pr-pipeline.yml` | PR pipeline. Same template, same scripts, same options; baseline = **`main` branch source**; **no Kusto ingestion**. | -| `sqlclient-perf-experiment-pipeline.yml` | Experiment pipeline. Same template, same scripts; both passes build the **same source** and differ only in one runner-config switch; **no Kusto ingestion**. | +| `sqlclient-perf-experiment.yml` | Experiment pipeline. Same template, same scripts; both passes build the **same source** and differ only in one runner-config switch; **no Kusto ingestion**. | | `scripts/run-perf-tests.sh` | Linux on-VM entry point: install SDK, create DB, run benchmarks (interleaved or sequential), compare. Baseline is a released package (`--baseline-version`), another git ref's source (`--baseline-source-ref`), or the same source with one runner-config switch flipped off (`--switch-under-test`). | | `scripts/run-perf-tests.ps1` | Windows equivalent (ProcessorAffinity instead of `taskset`). | | `scripts/interleave_perf.py` | Interleaved + best-of-N orchestrator: runs each unit baseline↔candidate back-to-back and confirms regressions across N passes. | @@ -152,7 +152,7 @@ the comparison (and one removed by the PR as `removed`) instead of failing the r share the single generated runner config (`RUNNER_CONFIG` / `DATATYPES_CONFIG` env vars), so connection string and behaviour flags are identical on both sides. -## Experiment pipeline (`sqlclient-perf-experiment-pipeline.yml`) +## Experiment pipeline (`sqlclient-perf-experiment.yml`) The three perf pipelines are the same benchmarks, template and scripts pointed at three different questions. Two of them vary the **source** under measurement; the third varies the **config**: @@ -161,9 +161,9 @@ questions. Two of them vary the **source** under measurement; the third varies t | --- | --- | --- | --- | | Has this branch regressed against a released package? | `sqlclient-perf-pipeline.yml` | released NuGet package | queued branch | | Does my PR regress the branch it merges into? | `sqlclient-perf-pr-pipeline.yml` | `main` source | queued branch | -| What does this switch cost or buy? | `sqlclient-perf-experiment-pipeline.yml` | queued branch, switch **off** | queued branch, switch **on** | +| What does this switch cost or buy? | `sqlclient-perf-experiment.yml` | queued branch, switch **off** | queued branch, switch **on** | -`sqlclient-perf-experiment-pipeline.yml` picks one runner-config switch via the `switchUnderTest` +`sqlclient-perf-experiment.yml` picks one runner-config switch via the `switchUnderTest` queue-time parameter (`UseConnectionPoolV2`, `UseOptimizedAsyncBehaviour` or `UseManagedSniOnWindows`) and runs the baseline pass with it `false` and the current pass with it `true`. Both passes measure the **same commit** — the branch the run is queued on — so queue it on a @@ -389,7 +389,7 @@ translated NDJSON as the `perf-kusto-payloads` artifact for manual/backfill inge ### Running the experiment pipeline -1. Open the **experiment** performance test pipeline (`sqlclient-perf-experiment-pipeline.yml`) in Azure +1. Open the **experiment** performance test pipeline (`sqlclient-perf-experiment.yml`) in Azure DevOps and select **Run pipeline**. 2. Choose the branch whose behaviour you want to measure — `main` to characterise the switch on its own, or a PR branch to characterise it against that change — and pick `switchUnderTest`. There is diff --git a/eng/pipelines/perf/sqlclient-perf-experiment-pipeline.yml b/eng/pipelines/perf/sqlclient-perf-experiment.yml similarity index 100% rename from eng/pipelines/perf/sqlclient-perf-experiment-pipeline.yml rename to eng/pipelines/perf/sqlclient-perf-experiment.yml From fcad5f8a6dd30cf8c1566e5e617db1aaeabdde80 Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Thu, 13 Aug 2026 16:19:22 -0700 Subject: [PATCH 04/34] Add inline fast path to ChannelDbConnectionPool.TryGetConnection Perf comparison of the v2 pool against v1 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 and only queues on a miss. - GetInternalConnection creates a timer-backed CancellationTokenSource before it knows whether it will wait, which also contends on the shared TimerQueue lock at higher parallelism. - GetInternalConnection is an async method, so it allocates a Task even when it completes synchronously. Add TryGetPooledConnectionInline, which performs the transacted-store and idle-channel lookups that GetInternalConnection starts with and returns null on a miss. Both entry points try it first, so the common case avoids the thread pool hop, the CTS, and the Task allocation. It never opens a physical connection, matching v1's allowCreate: false, so no caller thread blocks on network I/O. Does not address the sync SteadyStateOpenQueryClose regression, which comes from sync waiters serializing behind the process-wide _syncOverAsyncSemaphore. That semaphore bounds thread pool blocking process-wide and should not be made per-pool; removing the regression needs the sync-over-async channel wait redesigned. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../ConnectionPool/ChannelDbConnectionPool.cs | 120 +++++++++++++++++- .../ChannelDbConnectionPoolTest.cs | 81 ++++++++++++ 2 files changed, 200 insertions(+), 1 deletion(-) diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs index 9f75c50c1a..ecad9a7fef 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs @@ -850,11 +850,23 @@ public bool TryGetConnection( if (taskCompletionSource is null) { // We're on the caller's thread, so the ambient transaction is directly observable. + Transaction? currentTransaction = ADP.GetCurrentTransaction(); + + // Fast path: when the pool can satisfy the request immediately, do it here rather + // than entering GetInternalConnection, which allocates a Task + // and a timer-backed CancellationTokenSource before it knows whether it will ever + // need to wait. See TryGetPooledConnectionInline. + connection = TryGetPooledConnectionInline(owningObject, currentTransaction); + if (connection is not null) + { + return true; + } + var task = GetInternalConnection( owningObject, async: false, timeout, - ADP.GetCurrentTransaction()); + currentTransaction); // When running synchronously, we are guaranteed that the task is already completed. // We don't need to guard the managed threadpool at this spot because we pass the async flag as false @@ -909,6 +921,45 @@ public bool TryGetConnection( // processes pending opens on a dedicated non-thread-pool thread. Transaction? ambientTransaction = taskCompletionSource.Task.AsyncState as Transaction; + // Fast path: if the pool can satisfy this request right now, complete it on the + // caller's thread instead of dispatching to the thread pool. + // + // The Task.Run below costs a full thread pool dispatch plus a TaskCompletionSource + // handoff. That is unavoidable when we actually have to open a physical connection or + // wait for one to come back, but it dwarfs the work when an idle connection is already + // sitting in the channel - the common case for any pooled workload at steady state. + // + // This mirrors WaitHandleDbConnectionPool.TryGetConnection, whose async path also + // attempts an inline, non-blocking acquisition first (allowCreate: false) and only + // enqueues a PendingGetConnection when that misses. Like that path, we never open a + // physical connection inline, so the caller's thread is never blocked on network I/O. + // + // Exceptions are routed to the TaskCompletionSource rather than thrown synchronously, + // so the failure reaches the caller exactly as it would from the Task.Run path. + try + { + DbConnectionInternal? pooled = TryGetPooledConnectionInline(owningObject, ambientTransaction); + if (pooled is not null) + { + if (!taskCompletionSource.TrySetResult(pooled)) + { + // The request was cancelled out from under us; don't leak the connection. + ReturnInternalConnection(pooled, owningObject); + } + + connection = null; + return false; + } + } + catch (Exception e) + { + // TryGetPooledConnectionInline returns a connection to the pool itself if it fails + // after taking one, so there is nothing to release here. + taskCompletionSource.TrySetException(e); + connection = null; + return false; + } + Task.Run(async () => { if (taskCompletionSource.Task.IsCompleted) @@ -1264,6 +1315,73 @@ private void RemoveConnection(DbConnectionInternal connection) return null; } + /// + /// Attempts to satisfy a connection request from connections the pool already holds, + /// without blocking, waiting, or opening a physical connection. + /// + /// + /// + /// This is the fast path shared by the sync and async entry points of + /// . It performs the same two lookups the main loop in + /// begins with - the transacted store, then the idle + /// channel - and returns null the moment neither can satisfy the request, leaving the + /// caller to fall back to the full path. + /// + /// + /// Keeping this separate from is what makes it cheap. + /// That method is an async state machine that allocates a even + /// when it completes synchronously, and it creates a timer-backed + /// up front, before it knows whether it will ever + /// wait. Neither is needed to hand back a connection that is already sitting in the pool. + /// + /// + /// It deliberately does NOT call : that blocks on + /// network I/O, which must not happen on an async caller's thread. Callers that miss here + /// go through , which may open a connection. + /// + /// + /// The DbConnection that will own this internal connection. + /// The ambient transaction captured on the caller's thread, + /// or null when the caller is not inside a transaction. + /// An activated connection ready to be handed to the caller, or null when the pool + /// cannot satisfy the request without waiting or opening. + /// + /// Propagates any exception from activating or enlisting the connection. The connection is + /// returned to the pool before the exception escapes (see ). + /// + private DbConnectionInternal? TryGetPooledConnectionInline( + DbConnection owningConnection, + Transaction? ambientTransaction) + { + // When automatic enlistment is disabled the connection must never be bound to the + // ambient transaction, so we neither consult the transacted store nor hand the + // transaction to activation. Mirrors GetInternalConnection. + Transaction? transaction = HasTransactionAffinity ? ambientTransaction : null; + + DbConnectionInternal? connection = null; + + // A connection already enlisted in our transaction is always preferred, since reusing + // it avoids promoting the transaction to a distributed one. + if (transaction is not null) + { + connection = GetFromTransactedPool(transaction); + } + + // GetIdleConnection only returns connections that passed IsLiveConnection, and + // GetFromTransactedPool has already probed liveness, so no further validation is + // needed here. GetInternalConnection re-checks after its channel wait because that + // wait can hand back a connection that bypassed both filters. + connection ??= GetIdleConnection(); + + if (connection is null) + { + return null; + } + + PrepareConnection(owningConnection, connection, transaction); + return connection; + } + /// /// Gets an internal connection from the pool, either by retrieving an idle connection or opening a new one. /// diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTest.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTest.cs index 228e949cd0..a6417a2c74 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTest.cs @@ -233,6 +233,87 @@ out DbConnectionInternal? internalConnection Assert.Equal(pool.PoolGroupOptions.MaxPoolSize, pool.Count); } + /// + /// Verifies that an asynchronous request satisfied by an already-idle connection completes + /// inline on the caller's thread, rather than being dispatched to the thread pool. + /// + /// + /// The thread pool hop costs several microseconds against a pooled open that otherwise + /// takes a few, so it dominates steady-state async workloads. Asserting that the + /// TaskCompletionSource is already completed the instant TryGetConnection returns is what + /// pins the fast path down: if the request were dispatched via Task.Run, completion could + /// not be observed synchronously here. + /// + [Fact] + public async Task GetConnectionAsync_WithIdleConnection_ShouldCompleteInline() + { + // Arrange: take a connection and return it, leaving one connection idle in the pool. + var pool = ConstructPool(SuccessfulConnectionFactory); + SqlConnection owningConnection = new(); + + pool.TryGetConnection( + owningConnection, + taskCompletionSource: null, + TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)), + out DbConnectionInternal? pooledConnection + ); + Assert.NotNull(pooledConnection); + pool.ReturnInternalConnection(pooledConnection, owningConnection); + + // Act + TaskCompletionSource taskCompletionSource = new(); + var completed = pool.TryGetConnection( + new SqlConnection(), + taskCompletionSource, + TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)), + out DbConnectionInternal? internalConnection + ); + + // Assert: the async contract is unchanged - the caller is still told to await the + // TaskCompletionSource - but the result is already there, with no thread pool hop. + Assert.False(completed); + Assert.Null(internalConnection); + Assert.True(taskCompletionSource.Task.IsCompleted); + Assert.Equal(pooledConnection, await taskCompletionSource.Task); + + // The idle connection was reused rather than a second one being opened. + Assert.Equal(1, pool.Count); + } + + /// + /// Verifies that a synchronous request satisfied by an already-idle connection returns that + /// connection inline. + /// + [Fact] + public void GetConnection_WithIdleConnection_ShouldReturnInline() + { + // Arrange: take a connection and return it, leaving one connection idle in the pool. + var pool = ConstructPool(SuccessfulConnectionFactory); + SqlConnection owningConnection = new(); + + pool.TryGetConnection( + owningConnection, + taskCompletionSource: null, + TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)), + out DbConnectionInternal? pooledConnection + ); + Assert.NotNull(pooledConnection); + pool.ReturnInternalConnection(pooledConnection, owningConnection); + + // Act + var completed = pool.TryGetConnection( + new SqlConnection(), + taskCompletionSource: null, + TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)), + out DbConnectionInternal? internalConnection + ); + + // Assert + Assert.True(completed); + Assert.Equal(pooledConnection, internalConnection); + Assert.Equal(1, pool.Count); + } + /// /// Verifies that a waiting synchronous caller reuses a connection that is returned to an /// exhausted pool instead of creating a new physical connection. From 4c3058e40934049c770b45b51b71aa3d7f296873 Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Fri, 14 Aug 2026 12:31:34 -0700 Subject: [PATCH 05/34] Return the inline pooled connection instead of completing the TaskCompletionSource The first pass at the fast path completed the caller's TaskCompletionSource and returned false. That moved the thread pool dispatch rather than removing it: returning false sends SqlConnection.InternalOpenAsync down its asynchronous completion branch, which allocates an OpenAsyncRetry, a Tuple and a CancellationTokenRegistration, then schedules the continuation with ContinueWith(..., TaskScheduler.Default). That continuation costs a thread pool hop even though the result is already available. A re-run of the experiment pipeline showed the residual cost: OpenAsyncConnection was still +145% and RapidFireOpenClose still +23-58% with allocations up 70-163%, all of them async opens against an unsaturated pool that were hitting the fast path and paying for the handoff anyway. Return true with the connection instead, which is what WaitHandleDbConnectionPool does on its own inline hit, and leave the TaskCompletionSource untouched for the caller to abandon. InternalOpenAsync then takes its synchronous branch. Exceptions now propagate synchronously, which also matches v1; InternalOpenAsync already converts them into a faulted task. Update StressTestAsync, which awaited the TaskCompletionSource unconditionally and so hung once requests began completing inline. It now checks the completed flag first, matching the pattern already used in the pool transaction tests and by the pool's real callers. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../ConnectionPool/ChannelDbConnectionPool.cs | 54 ++++++++----------- .../ChannelDbConnectionPoolTest.cs | 47 ++++++++++------ 2 files changed, 52 insertions(+), 49 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs index ecad9a7fef..e5b543e60c 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs @@ -921,43 +921,31 @@ public bool TryGetConnection( // processes pending opens on a dedicated non-thread-pool thread. Transaction? ambientTransaction = taskCompletionSource.Task.AsyncState as Transaction; - // Fast path: if the pool can satisfy this request right now, complete it on the - // caller's thread instead of dispatching to the thread pool. + // Fast path: if the pool can satisfy this request right now, hand the connection back + // synchronously and report the request as completed. // - // The Task.Run below costs a full thread pool dispatch plus a TaskCompletionSource - // handoff. That is unavoidable when we actually have to open a physical connection or - // wait for one to come back, but it dwarfs the work when an idle connection is already - // sitting in the channel - the common case for any pooled workload at steady state. + // Returning true here (rather than completing the TaskCompletionSource and returning + // false) is what makes this cheap, and it is what + // WaitHandleDbConnectionPool.TryGetConnection does on its own inline hit. Reporting the + // open as incomplete sends SqlConnection.InternalOpenAsync down its asynchronous + // completion path, which allocates an OpenAsyncRetry, a CancellationTokenRegistration + // and a Tuple, and then schedules the continuation with + // ContinueWith(..., TaskScheduler.Default). That continuation costs a thread pool + // dispatch even when the TaskCompletionSource is already completed, so completing it + // inline would move the dispatch rather than remove it. Returning true instead lets + // InternalOpenAsync take its synchronous branch and skip all of it. // - // This mirrors WaitHandleDbConnectionPool.TryGetConnection, whose async path also - // attempts an inline, non-blocking acquisition first (allowCreate: false) and only - // enqueues a PendingGetConnection when that misses. Like that path, we never open a - // physical connection inline, so the caller's thread is never blocked on network I/O. + // The TaskCompletionSource is deliberately left untouched. The caller abandons it when + // the open completes synchronously, exactly as it does for the WaitHandle pool. // - // Exceptions are routed to the TaskCompletionSource rather than thrown synchronously, - // so the failure reaches the caller exactly as it would from the Task.Run path. - try - { - DbConnectionInternal? pooled = TryGetPooledConnectionInline(owningObject, ambientTransaction); - if (pooled is not null) - { - if (!taskCompletionSource.TrySetResult(pooled)) - { - // The request was cancelled out from under us; don't leak the connection. - ReturnInternalConnection(pooled, owningObject); - } - - connection = null; - return false; - } - } - catch (Exception e) + // Like v1's inline attempt (allowCreate: false), this never opens a physical + // connection, so the caller's thread is never blocked on network I/O. Exceptions + // propagate synchronously, which is also what the WaitHandle pool does from here. + DbConnectionInternal? pooled = TryGetPooledConnectionInline(owningObject, ambientTransaction); + if (pooled is not null) { - // TryGetPooledConnectionInline returns a connection to the pool itself if it fails - // after taking one, so there is nothing to release here. - taskCompletionSource.TrySetException(e); - connection = null; - return false; + connection = pooled; + return true; } Task.Run(async () => diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTest.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTest.cs index a6417a2c74..2cd8867345 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTest.cs @@ -234,18 +234,20 @@ out DbConnectionInternal? internalConnection } /// - /// Verifies that an asynchronous request satisfied by an already-idle connection completes - /// inline on the caller's thread, rather than being dispatched to the thread pool. + /// Verifies that an asynchronous request satisfied by an already-idle connection is + /// completed inline on the caller's thread, rather than being dispatched to the thread pool. /// /// - /// The thread pool hop costs several microseconds against a pooled open that otherwise - /// takes a few, so it dominates steady-state async workloads. Asserting that the - /// TaskCompletionSource is already completed the instant TryGetConnection returns is what - /// pins the fast path down: if the request were dispatched via Task.Run, completion could - /// not be observed synchronously here. + /// The fast path must report completion by returning true with the connection, exactly as + /// WaitHandleDbConnectionPool does on its inline hit. Completing the TaskCompletionSource + /// and returning false would look equivalent but is not: it sends + /// SqlConnection.InternalOpenAsync down its asynchronous branch, which allocates an + /// OpenAsyncRetry and schedules ContinueWith(..., TaskScheduler.Default), costing a thread + /// pool dispatch even though the result is already available. Asserting that the + /// TaskCompletionSource is left untouched is what pins that down. /// [Fact] - public async Task GetConnectionAsync_WithIdleConnection_ShouldCompleteInline() + public void GetConnectionAsync_WithIdleConnection_ShouldCompleteInline() { // Arrange: take a connection and return it, leaving one connection idle in the pool. var pool = ConstructPool(SuccessfulConnectionFactory); @@ -269,12 +271,16 @@ out DbConnectionInternal? pooledConnection out DbConnectionInternal? internalConnection ); - // Assert: the async contract is unchanged - the caller is still told to await the - // TaskCompletionSource - but the result is already there, with no thread pool hop. - Assert.False(completed); - Assert.Null(internalConnection); - Assert.True(taskCompletionSource.Task.IsCompleted); - Assert.Equal(pooledConnection, await taskCompletionSource.Task); + // Assert: the request is reported as completed and the connection is handed back + // directly, matching WaitHandleDbConnectionPool's inline hit. This is what lets + // SqlConnection.InternalOpenAsync take its synchronous branch and skip the + // OpenAsyncRetry allocation and the ContinueWith thread pool dispatch. + Assert.True(completed); + Assert.Equal(pooledConnection, internalConnection); + + // The TaskCompletionSource must be left alone; the caller abandons it on a + // synchronous completion. + Assert.False(taskCompletionSource.Task.IsCompleted); // The idle connection was reused rather than a second one being opened. Assert.Equal(1, pool.Count); @@ -707,10 +713,19 @@ public void StressTestAsync() TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)), out DbConnectionInternal? internalConnection ); - internalConnection = await taskCompletionSource.Task; - pool.ReturnInternalConnection(internalConnection, owningObject); + + // A request satisfied from the pool's existing connections completes inline, + // returning the connection directly and leaving the TaskCompletionSource + // untouched. Only fall back to awaiting it when the request was handed off. + // This mirrors how the pool's callers consume TryGetConnection. + if (!completed) + { + internalConnection = await taskCompletionSource.Task; + } Assert.NotNull(internalConnection); + + pool.ReturnInternalConnection(internalConnection, owningObject); }); tasks.Add(t); } From 7a24b392b5a2e63754d0fd43644f079db325ecd9 Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Mon, 17 Aug 2026 10:25:25 -0700 Subject: [PATCH 06/34] Annotate expected differences in switch experiments A switch experiment flips intended behaviour, so benchmarks that measure that behaviour regress by design. UseConnectionPoolV2 is the motivating case: ChannelDbConnectionPool opens physical connections concurrently, where WaitHandleDbConnectionPool serialises growth behind a Semaphore(1, 1), and ConnectionPoolStressRunner.RapidFireOpenClose measures a cold-start burst where the extra parallel opens have nothing to amortise against. With nowhere to record that, the same benchmarks get re-investigated every run and --fail-on-regression is unusable for experiments. Add an optional per-switch annotation file at expected-differences/.json, picked up automatically by --switch-under-test. Matching entries are reported as expected differences, grouped under their reason in comparison.md, excluded from the confirmed regression count and ignored by the regression gate. Two limits keep the annotation honest: a reason is required, and only regressions are reclassified, so an annotated benchmark that comes back unchanged or improved keeps its real status. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- eng/pipelines/perf/README.md | 44 ++++- .../UseConnectionPoolV2.json | 8 + eng/pipelines/perf/scripts/interleave_perf.py | 159 +++++++++++++++++- eng/pipelines/perf/scripts/run-perf-tests.ps1 | 12 ++ eng/pipelines/perf/scripts/run-perf-tests.sh | 11 ++ 5 files changed, 225 insertions(+), 9 deletions(-) create mode 100644 eng/pipelines/perf/expected-differences/UseConnectionPoolV2.json diff --git a/eng/pipelines/perf/README.md b/eng/pipelines/perf/README.md index 731d75cf5d..ea9a253ac7 100644 --- a/eng/pipelines/perf/README.md +++ b/eng/pipelines/perf/README.md @@ -15,7 +15,8 @@ database. | `sqlclient-perf-experiment.yml` | Experiment pipeline. Same template, same scripts; both passes build the **same source** and differ only in one runner-config switch; **no Kusto ingestion**. | | `scripts/run-perf-tests.sh` | Linux on-VM entry point: install SDK, create DB, run benchmarks (interleaved or sequential), compare. Baseline is a released package (`--baseline-version`), another git ref's source (`--baseline-source-ref`), or the same source with one runner-config switch flipped off (`--switch-under-test`). | | `scripts/run-perf-tests.ps1` | Windows equivalent (ProcessorAffinity instead of `taskset`). | -| `scripts/interleave_perf.py` | Interleaved + best-of-N orchestrator: runs each unit baseline↔candidate back-to-back and confirms regressions across N passes. | +| `scripts/interleave_perf.py` | Interleaved + best-of-N orchestrator: runs each unit baseline↔candidate back-to-back and confirms regressions across N passes. Also applies the expected-difference annotations. | +| `expected-differences/.json` | Per-switch annotations naming benchmarks whose regression is an intended consequence of that switch, each with a required reason. Picked up automatically by `--switch-under-test`. | | `scripts/compare_perf.py` | Compares baseline vs current BenchmarkDotNet JSON → delta (md + json). Reused by the orchestrator. | | `scripts/perf_to_kusto.py` | Translates BenchmarkDotNet "full" JSON → Kusto `PerfRun` + `PerfBenchmarkResult` NDJSON. | | `scripts/ingest_kusto.py` | Queued Kusto ingestion (az CLI auth, runs on the agent). | @@ -212,6 +213,47 @@ experiment would corrupt the perf database three ways: The comparison report and the raw BenchmarkDotNet artifacts are published as usual, and the build is tagged `Switch ` so experiments are identifiable in the ADO build list. +### Expected differences + +A switch experiment flips intended behaviour, so any benchmark that measures that behaviour regresses +by design. The `UseConnectionPoolV2` case is the motivating example: `ChannelDbConnectionPool` opens +physical connections concurrently, where `WaitHandleDbConnectionPool` serialises growth behind a +`Semaphore(1, 1)`. `ConnectionPoolStressRunner.RapidFireOpenClose` calls `ClearAllPools()` in +`[IterationCleanup]` and holds connections for zero time, so it measures a cold-start burst in which +the extra parallel opens have nothing to amortise against. That regression is the trade-off working +as intended, not a defect. + +Without somewhere to record that, the same benchmarks get re-investigated every run and +`--fail-on-regression` is unusable for experiments. Each switch may therefore have an annotation file +at `expected-differences/.json`, picked up automatically by +`--switch-under-test` / `-SwitchUnderTest` when present: + +```json +[ + { + "benchmark": "ConnectionPoolStressRunner", + "method": "RapidFireOpenClose", + "params": "*", + "reason": "Why this difference is intended, with the evidence behind that conclusion." + } +] +``` + +`benchmark`, `method` and `params` are shell-style globs, each defaulting to `*`. `reason` is +required and the file is rejected without it, so an annotation is always reviewable rather than a +silent mute. + +Matching entries are reported as `🔵 expected difference`, listed in their own section of +`comparison.md` under the reason, excluded from the confirmed-regression count and ignored by the +regression gate. Two deliberate limits keep this honest: + +* Only regressions are reclassified. An annotated benchmark that comes back **unchanged or improved** + keeps its real status, so an annotation can never hide a result getting better. +* The best-of-N confirmation count is still computed first, so an expected difference that stops + reproducing is still visible in the JSON. + +A switch with no annotation file simply reports every regression as a regression. + ## Two-pass build model The `PerformanceTests` project references Microsoft.Data.SqlClient two ways, selected by MSBuild: diff --git a/eng/pipelines/perf/expected-differences/UseConnectionPoolV2.json b/eng/pipelines/perf/expected-differences/UseConnectionPoolV2.json new file mode 100644 index 0000000000..953ac3f632 --- /dev/null +++ b/eng/pipelines/perf/expected-differences/UseConnectionPoolV2.json @@ -0,0 +1,8 @@ +[ + { + "benchmark": "ConnectionPoolStressRunner", + "method": "RapidFireOpenClose", + "params": "*", + "reason": "ChannelDbConnectionPool opens physical connections concurrently by design; WaitHandleDbConnectionPool serialises growth behind a Semaphore(1, 1) and its WaitAny prefers a freed connection over a creation slot. This benchmark calls ClearAllPools() in [IterationCleanup] and holds connections for zero time, so every iteration measures a cold-start burst where the extra parallel opens have nothing to amortise against. Measured with a counting connection factory at Parallelism=20/MaxPoolSize=50: v1 creates 1 physical connection, v2 creates 9. Serialising v2 growth removes the regression entirely, which is the behaviour v2 exists to avoid. Steady-state churn already favours v2 (RapidOpenCloseSingleThreadAsync -15.5%, MultiCommandReuse -8% to -23%)." + } +] diff --git a/eng/pipelines/perf/scripts/interleave_perf.py b/eng/pipelines/perf/scripts/interleave_perf.py index 6522adced1..cd40acdff2 100644 --- a/eng/pipelines/perf/scripts/interleave_perf.py +++ b/eng/pipelines/perf/scripts/interleave_perf.py @@ -27,6 +27,7 @@ """ import argparse +import fnmatch import glob import json import os @@ -40,6 +41,94 @@ import compare_perf # noqa: E402 (local module, sits next to this script) +# -------------------------------------------------------------------------------------------------- +# Expected differences (switch experiments). +# -------------------------------------------------------------------------------------------------- +# A switch experiment compares the same build with one AppContext switch flipped, so any *intended* +# behavioural difference shows up as a regression. Example: UseConnectionPoolV2 deliberately allows +# concurrent physical connection opens, where the v1 pool serialises them behind a Semaphore(1, 1). +# Benchmarks that open connections in a burst against a cold pool therefore measure v2 creating more +# connections, which is the intended trade-off rather than a defect. +# +# Without a way to record that, every run re-litigates the same benchmarks and the --fail-on-regression +# gate is unusable for experiments. An expected-differences file names those benchmarks and, crucially, +# requires a reason, so the annotation is reviewable rather than a silent mute. + +def load_expected_differences(path): + """Loads an expected-differences file. + + The file is a JSON list of rules:: + + [ + { + "benchmark": "ConnectionPoolStressRunner", + "method": "RapidFireOpenClose", + "params": "*", + "reason": "v2 opens connections concurrently by design; ..." + } + ] + + ``benchmark``, ``method`` and ``params`` are shell-style globs and each defaults to ``*``. + ``reason`` is required: an unexplained entry is indistinguishable from hiding a real regression. + """ + with open(path, "r", encoding="utf-8") as handle: + raw = json.load(handle) + + if not isinstance(raw, list): + raise ValueError(f"{path}: expected a JSON list of rules, got {type(raw).__name__}.") + + rules = [] + for index, item in enumerate(raw): + if not isinstance(item, dict): + raise ValueError(f"{path}: rule {index} is not an object.") + reason = (item.get("reason") or "").strip() + if not reason: + raise ValueError( + f"{path}: rule {index} is missing a non-empty 'reason'. Every expected difference " + "must say why it is expected." + ) + rules.append({ + "benchmark": item.get("benchmark", "*"), + "method": item.get("method", "*"), + "params": item.get("params", "*"), + "reason": reason, + }) + return rules + + +def _rule_matches(rule, entry): + return ( + fnmatch.fnmatch(entry.get("benchmarkName") or "", rule["benchmark"]) + and fnmatch.fnmatch(entry.get("methodName") or "", rule["method"]) + and fnmatch.fnmatch(entry.get("parameterSignature") or "", rule["params"]) + ) + + +def apply_expected_differences(entries, rules): + """Reclassifies regressions that a rule marks as intended. + + Runs *after* the best-of-N verdict so the confirmation count is still recorded: an expected + difference that stops reproducing is itself worth noticing. Only regressions are reclassified; + an annotated benchmark that comes back unchanged or improved keeps its real status, so the + annotation cannot mask a result getting better. + """ + if not rules: + return [] + + annotated = [] + for entry in entries: + if entry["status"] not in ("regression", "regression-unconfirmed"): + continue + for rule in rules: + if _rule_matches(rule, entry): + entry["status"] = "expected-difference" + entry["confirmedRegression"] = False + entry["expectedReason"] = rule["reason"] + annotated.append(entry) + break + return annotated + + # -------------------------------------------------------------------------------------------------- # CPU affinity (client pinning, wiki §2.4) — best effort, cross platform. # -------------------------------------------------------------------------------------------------- @@ -234,7 +323,7 @@ def _regression_keys(baseline_dir, current_dir, threshold): return entries, {e["key"] for e in entries if e["status"] == "regression"} -def orchestrate(runner, units, results_dir, threshold, reps): +def orchestrate(runner, units, results_dir, threshold, reps, expected_rules=None): baseline_agg = os.path.join(results_dir, "baseline") current_agg = os.path.join(results_dir, "current") comparison_dir = os.path.join(results_dir, "comparison") @@ -290,10 +379,12 @@ def orchestrate(runner, units, results_dir, threshold, reps): e["totalReps"] = total_reps e["confirmedRegression"] = False + # Applied after the verdict so an intended difference still carries its confirmation count. + expected = apply_expected_differences(entries, expected_rules) + confirmed = [e for e in entries if e.get("confirmedRegression")] unconfirmed = [e for e in entries if e["status"] == "regression-unconfirmed"] - return entries, confirmed, unconfirmed - + return entries, confirmed, unconfirmed, expected # -------------------------------------------------------------------------------------------------- # Rendering. @@ -301,6 +392,7 @@ def orchestrate(runner, units, results_dir, threshold, reps): _ICON = { "regression": "🔴 regression", "regression-unconfirmed": "🟠 regression (unconfirmed)", + "expected-difference": "🔵 expected difference", "improvement": "🟢 improvement", "unchanged": "⚪ unchanged", "current-only": "🆕 new", @@ -309,7 +401,7 @@ def orchestrate(runner, units, results_dir, threshold, reps): } -def render_markdown(entries, confirmed, unconfirmed, baseline_version, threshold, reps): +def render_markdown(entries, confirmed, unconfirmed, expected, baseline_version, threshold, reps): improvements = [e for e in entries if e["status"] == "improvement"] lines = [] lines.append("# SqlClient Performance Comparison (interleaved, best-of-N)") @@ -322,6 +414,9 @@ def render_markdown(entries, confirmed, unconfirmed, baseline_version, threshold lines.append(f"- **Confirmed** regressions (majority of {reps}): **{len(confirmed)}**") lines.append(f"- Unconfirmed (flagged once, not reproduced): **{len(unconfirmed)}**") lines.append(f"- Improvements (faster > {threshold:.0f}%): **{len(improvements)}**") + if expected: + lines.append(f"- Expected differences (annotated, not counted as regressions): " + f"**{len(expected)}**") lines.append("") lines.append("| Status | Benchmark | Method | Params | Baseline (ms) | " "Current (ms) | Mean Δ | Alloc Δ | Confirm |") @@ -347,6 +442,37 @@ def render_markdown(entries, confirmed, unconfirmed, baseline_version, threshold ) ) lines.append("") + + if expected: + lines.append("## Expected differences") + lines.append("") + lines.append("These benchmarks regressed, but the annotation file records the regression as an " + "intended consequence of the switch under test. They are excluded from the " + "confirmed-regression count and from the regression gate.") + lines.append("") + # Grouped by reason: one rule usually covers a whole family of parameterisations, and + # repeating a paragraph of rationale per row buries it. + by_reason = {} + for e in expected: + by_reason.setdefault(e.get("expectedReason", ""), []).append(e) + + for reason, group in by_reason.items(): + lines.append(reason) + lines.append("") + lines.append("| Benchmark | Method | Params | Mean Δ | Alloc Δ |") + lines.append("| --------- | ------ | ------ | ------ | ------- |") + for e in group: + lines.append( + "| {name} | {method} | {params} | {delta} | {alloc} |".format( + name=e["benchmarkName"], + method=e["methodName"], + params=e["parameterSignature"] or "-", + delta=compare_perf._fmt_pct(e["meanDeltaPct"]), + alloc=compare_perf._fmt_pct(e["allocDeltaPct"]), + ) + ) + lines.append("") + return "\n".join(lines) @@ -378,6 +504,11 @@ def main(argv=None): help="Override RUNNER_CONFIG for current-variant subprocesses only.") parser.add_argument("--client-cpus", default=os.environ.get("PERF_CLIENT_CPUS", ""), help="CPU set to pin the benchmark client to, e.g. '16-31'.") + parser.add_argument("--expected-differences", default=None, + help="Path to a JSON file naming benchmarks whose regression is an intended " + "consequence of the switch under test. Matching entries are reported " + "separately and do not trip --fail-on-regression. Each rule must carry " + "a reason.") parser.add_argument("--fail-on-regression", action="store_true", help="Exit non-zero if any CONFIRMED regression is detected.") args = parser.parse_args(argv) @@ -403,18 +534,29 @@ def main(argv=None): baseline_runner_config=args.baseline_runner_config, current_runner_config=args.current_runner_config) + expected_rules = [] + if args.expected_differences: + try: + expected_rules = load_expected_differences(args.expected_differences) + except (OSError, ValueError, json.JSONDecodeError) as exc: + print(f"ERROR: could not load --expected-differences: {exc}", file=sys.stderr) + return 1 + print(f"Loaded {len(expected_rules)} expected-difference rule(s) from " + f"{args.expected_differences}.") + units = runner.list_units() if not units: print("ERROR: no enabled benchmark units were reported by the current build.", file=sys.stderr) return 1 print(f"Enabled units ({len(units)}): {', '.join(units)}") - entries, confirmed, unconfirmed = orchestrate( - runner, units, results_dir, args.threshold, args.reps) + entries, confirmed, unconfirmed, expected = orchestrate( + runner, units, results_dir, args.threshold, args.reps, expected_rules) # Outputs. comparison_dir = os.path.join(results_dir, "comparison") - md = render_markdown(entries, confirmed, unconfirmed, args.baseline_version, args.threshold, args.reps) + md = render_markdown(entries, confirmed, unconfirmed, expected, + args.baseline_version, args.threshold, args.reps) with open(os.path.join(comparison_dir, "comparison.md"), "w", encoding="utf-8") as fh: fh.write(md + "\n") with open(os.path.join(comparison_dir, "comparison.json"), "w", encoding="utf-8") as fh: @@ -424,6 +566,7 @@ def main(argv=None): "confirmationRuns": args.reps, "confirmedRegressions": len(confirmed), "unconfirmedRegressions": len(unconfirmed), + "expectedDifferences": len(expected), "entries": entries, }, fh, indent=2) # Surface the comparison as the top-level run summary (collect-results attaches results/*.md). @@ -431,7 +574,7 @@ def main(argv=None): os.path.join(results_dir, "summary.md")) print(f"Interleaved comparison complete: {len(confirmed)} confirmed regression(s), " - f"{len(unconfirmed)} unconfirmed.") + f"{len(unconfirmed)} unconfirmed, {len(expected)} expected difference(s).") if args.fail_on_regression and confirmed: print("Confirmed regressions detected; failing as requested.", file=sys.stderr) diff --git a/eng/pipelines/perf/scripts/run-perf-tests.ps1 b/eng/pipelines/perf/scripts/run-perf-tests.ps1 index bf740e61eb..0b14150be7 100644 --- a/eng/pipelines/perf/scripts/run-perf-tests.ps1 +++ b/eng/pipelines/perf/scripts/run-perf-tests.ps1 @@ -790,6 +790,18 @@ if ((-not [string]::IsNullOrEmpty($BaselineLabel)) -and ($RunMode -eq "interleav # flavour keeps sharing the single ambient RUNNER_CONFIG set above. if (-not [string]::IsNullOrEmpty($SwitchUnderTest)) { $interleaveArgs += @("--baseline-runner-config", $BaselineRunnerConfig, "--current-runner-config", $CurrentRunnerConfig) + + # A switch experiment flips intended behaviour, so benchmarks that measure that behaviour + # regress by design. If the switch has an annotation file, hand it to the comparison so those + # show up as expected differences instead of being re-litigated every run. Optional: a switch + # with no annotated benchmarks simply has no file. + $ExpectedDifferencesFile = Join-Path (Split-Path -Parent $ScriptDir) (Join-Path "expected-differences" "$SwitchUnderTest.json") + if (Test-Path $ExpectedDifferencesFile) { + Write-Host "Using expected-differences annotations: $ExpectedDifferencesFile" + $interleaveArgs += @("--expected-differences", $ExpectedDifferencesFile) + } else { + Write-Host "No expected-differences file for $SwitchUnderTest; all regressions will be reported as such." + } } if ($FailOnRegression) { Write-Host "Regression gate ENABLED: a CONFIRMED candidate-slower regression (> $RegressionThreshold%) will fail the run." diff --git a/eng/pipelines/perf/scripts/run-perf-tests.sh b/eng/pipelines/perf/scripts/run-perf-tests.sh index 70ed796b4a..a370761744 100755 --- a/eng/pipelines/perf/scripts/run-perf-tests.sh +++ b/eng/pipelines/perf/scripts/run-perf-tests.sh @@ -805,6 +805,17 @@ if [[ -n "${baselineLabel}" && "${runMode}" == "interleaved" ]]; then --baseline-runner-config "${BASELINE_RUNNER_CONFIG}" --current-runner-config "${CURRENT_RUNNER_CONFIG}" ) + # A switch experiment flips intended behaviour, so benchmarks that measure that behaviour + # regress by design. If the switch has an annotation file, hand it to the comparison so those + # show up as expected differences instead of being re-litigated every run. Optional: a switch + # with no annotated benchmarks simply has no file. + expectedDifferencesFile="${SCRIPT_DIR}/../expected-differences/${switchUnderTest}.json" + if [[ -f "${expectedDifferencesFile}" ]]; then + echo "Using expected-differences annotations: ${expectedDifferencesFile}" + interleave_args+=(--expected-differences "${expectedDifferencesFile}") + else + echo "No expected-differences file for ${switchUnderTest}; all regressions will be reported as such." + fi fi if [[ "${failOnRegression}" == "true" ]]; then echo "Regression gate ENABLED: a CONFIRMED candidate-slower regression (> ${regressionThreshold}%) will fail the run." From ca0e52e02229a28dd08e22f0bab9aada45e9c20c Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Mon, 17 Aug 2026 11:32:58 -0700 Subject: [PATCH 07/34] Replace sync-over-async channel wait and add a cold-start ramp benchmark Sync callers on ChannelDbConnectionPool waited for an idle connection by driving Channel.ReadAsync from a blocked thread, so the wait could only be released by a thread pool continuation. Blocked sync callers occupy the very threads that continuation needs, so a static SemaphoreSlim sized ProcessorCount/2 capped how many sync callers could wait at once. That guard is process-wide and shared across every pool. IdleConnectionChannel now gates reads on a counting SemaphoreSlim, released directly by the writing thread. Sync callers block on SemaphoreSlim.Wait, which needs no continuation, so the starvation risk and the throughput cap both go away. Async callers gate on the same semaphore to keep the counts aligned, and channel completion is surfaced through a linked token so waiters still see ChannelClosedException. ConnectionPoolRampRunner covers what RapidFireOpenClose cannot. That benchmark starts cold but releases each connection immediately, so one physical connection satisfies every caller and slow pool growth wins. The new runner keeps the cold pool and holds each connection until all callers have one, so the pool must open N connections and concurrent creation is what is measured. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- eng/pipelines/perf/README.md | 7 + .../UseConnectionPoolV2.json | 2 +- .../ConnectionPool/ChannelDbConnectionPool.cs | 41 +---- .../ConnectionPool/IdleConnectionChannel.cs | 67 +++++++- .../ConnectionPoolRampRunner.cs | 150 ++++++++++++++++++ .../tests/PerformanceTests/Config/Config.cs | 1 + .../tests/PerformanceTests/Program.cs | 1 + .../tests/PerformanceTests/runnerconfig.jsonc | 8 + .../IdleConnectionChannelTest.cs | 92 +++++++++++ 9 files changed, 325 insertions(+), 44 deletions(-) create mode 100644 src/Microsoft.Data.SqlClient/tests/PerformanceTests/BenchmarkRunners/ConnectionPoolRampRunner.cs diff --git a/eng/pipelines/perf/README.md b/eng/pipelines/perf/README.md index ea9a253ac7..ccf6428d1d 100644 --- a/eng/pipelines/perf/README.md +++ b/eng/pipelines/perf/README.md @@ -223,6 +223,13 @@ physical connections concurrently, where `WaitHandleDbConnectionPool` serialises the extra parallel opens have nothing to amortise against. That regression is the trade-off working as intended, not a defect. +An annotation is the fallback, not the first answer. Where a benchmark can be written so it measures +the intended behaviour directly, prefer that. `ConnectionPoolRampRunner` was added for exactly this +reason: it keeps the cold pool but makes every caller *hold* its connection until all of them have +connected, so the pool genuinely needs N physical connections and the only variable left is how fast +it can open them. That rewards concurrent creation instead of penalising it, and it is the case +`RapidFireOpenClose` cannot express. + Without somewhere to record that, the same benchmarks get re-investigated every run and `--fail-on-regression` is unusable for experiments. Each switch may therefore have an annotation file at `expected-differences/.json`, picked up automatically by diff --git a/eng/pipelines/perf/expected-differences/UseConnectionPoolV2.json b/eng/pipelines/perf/expected-differences/UseConnectionPoolV2.json index 953ac3f632..4ad02fe02c 100644 --- a/eng/pipelines/perf/expected-differences/UseConnectionPoolV2.json +++ b/eng/pipelines/perf/expected-differences/UseConnectionPoolV2.json @@ -3,6 +3,6 @@ "benchmark": "ConnectionPoolStressRunner", "method": "RapidFireOpenClose", "params": "*", - "reason": "ChannelDbConnectionPool opens physical connections concurrently by design; WaitHandleDbConnectionPool serialises growth behind a Semaphore(1, 1) and its WaitAny prefers a freed connection over a creation slot. This benchmark calls ClearAllPools() in [IterationCleanup] and holds connections for zero time, so every iteration measures a cold-start burst where the extra parallel opens have nothing to amortise against. Measured with a counting connection factory at Parallelism=20/MaxPoolSize=50: v1 creates 1 physical connection, v2 creates 9. Serialising v2 growth removes the regression entirely, which is the behaviour v2 exists to avoid. Steady-state churn already favours v2 (RapidOpenCloseSingleThreadAsync -15.5%, MultiCommandReuse -8% to -23%)." + "reason": "ChannelDbConnectionPool opens physical connections concurrently by design; WaitHandleDbConnectionPool serialises growth behind a Semaphore(1, 1) and its WaitAny prefers a freed connection over a creation slot. This benchmark calls ClearAllPools() in [IterationCleanup] and holds connections for zero time, so every iteration measures a cold-start burst where the extra parallel opens have nothing to amortise against. Measured with a counting connection factory at Parallelism=20/MaxPoolSize=50: v1 creates 1 physical connection, v2 creates 9. Serialising v2 growth removes the regression entirely, which is the behaviour v2 exists to avoid. Steady-state churn already favours v2 (RapidOpenCloseSingleThreadAsync -15.5%, MultiCommandReuse -8% to -23%), and ConnectionPoolRampRunner measures the same cold burst with connections held, which is where concurrent creation is supposed to pay off." } ] diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs index e5b543e60c..ca019119de 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs @@ -60,12 +60,6 @@ namespace Microsoft.Data.SqlClient.ConnectionPool internal sealed class ChannelDbConnectionPool : IDbConnectionPool, IDisposable { #region Fields - // Limits synchronous operations which depend on async operations on managed - // threads from blocking on all available threads, which would stop async tasks - // from being scheduled and cause deadlocks. Use ProcessorCount/2 as a balance - // between sync and async tasks. - private static SemaphoreSlim _syncOverAsyncSemaphore = new(Math.Max(1, Environment.ProcessorCount / 2)); - /// /// Tracks the number of instances of this class. Used to generate unique IDs for each instance. /// @@ -1455,7 +1449,7 @@ private async Task GetInternalConnection( } else { - connection ??= ReadChannelSyncOverAsync(cancellationToken); + connection ??= _idleChannel.Read(cancellationToken); } } catch (OperationCanceledException) @@ -1479,39 +1473,6 @@ private async Task GetInternalConnection( return connection; } - /// - /// Performs a blocking synchronous read from the idle connection channel. - /// - /// Cancels the read operation. - /// The connection read from the channel. - private DbConnectionInternal? ReadChannelSyncOverAsync(CancellationToken cancellationToken) - { - // If there are no connections in the channel, then ReadAsync will block until one is available. - // Channels doesn't offer a sync API, so running ReadAsync synchronously on this thread may spawn - // additional new async work items in the managed thread pool if there are no items available in the - // channel. We need to ensure that we don't block all available managed threads with these child - // tasks or we could deadlock. Prefer to block the current user-owned thread, and limit throughput - // to the managed threadpool. - - _syncOverAsyncSemaphore.Wait(cancellationToken); - try - { - ConfiguredValueTaskAwaitable.ConfiguredValueTaskAwaiter awaiter = - _idleChannel.ReadAsync(cancellationToken).ConfigureAwait(false).GetAwaiter(); - using ManualResetEventSlim mres = new ManualResetEventSlim(false, 0); - - // Cancellation happens through the ReadAsync call, which will complete the task. - // Even a failed task will complete and set the ManualResetEventSlim. - awaiter.UnsafeOnCompleted(() => mres.Set()); - mres.Wait(CancellationToken.None); - return awaiter.GetResult(); - } - finally - { - _syncOverAsyncSemaphore.Release(); - } - } - /// /// Sets connection state and activates the connection for use. Should always be called after a connection is /// created or retrieved from the pool. diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/IdleConnectionChannel.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/IdleConnectionChannel.cs index 348ad33d9e..8d2d5b953f 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/IdleConnectionChannel.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/IdleConnectionChannel.cs @@ -1,6 +1,7 @@ // Licensed to the .NET Foundation under one or more agreements. // The .NET Foundation licenses this file to you under the MIT license. // See the LICENSE file in the project root for more information. +using System; using System.Threading; using System.Threading.Tasks; using System.Threading.Channels; @@ -21,6 +22,8 @@ internal sealed class IdleConnectionChannel private readonly ChannelReader _reader; private readonly ChannelWriter _writer; private volatile int _count; + private readonly SemaphoreSlim _items = new(0); + private readonly CancellationTokenSource _completedCts = new(); internal IdleConnectionChannel() { @@ -37,7 +40,12 @@ internal IdleConnectionChannel() /// /// if this call completed the channel; otherwise /// (channel was already completed). - internal bool Complete() => _writer.TryComplete(); + internal bool Complete() + { + bool completed = _writer.TryComplete(); + _completedCts.Cancel(); + return completed; + } /// /// The number of non-null connections currently in the channel. @@ -57,6 +65,7 @@ internal bool TryWrite(DbConnectionInternal? connection) { Interlocked.Increment(ref _count); } + _items.Release(); return true; } @@ -69,7 +78,8 @@ internal bool TryWrite(DbConnectionInternal? connection) /// internal bool TryRead(out DbConnectionInternal? connection) { - if (_reader.TryRead(out connection)) + connection = null; + if (_items.Wait(0) && _reader.TryRead(out connection)) { if (connection is not null) { @@ -88,7 +98,58 @@ internal bool TryRead(out DbConnectionInternal? connection) /// internal async ValueTask ReadAsync(CancellationToken cancellationToken) { - var connection = await _reader.ReadAsync(cancellationToken).ConfigureAwait(false); + if (!_items.Wait(0)) + { + using var linkedAsync = CancellationTokenSource.CreateLinkedTokenSource( + cancellationToken, _completedCts.Token); + try + { + await _items.WaitAsync(linkedAsync.Token).ConfigureAwait(false); + } + catch (OperationCanceledException) when (!cancellationToken.IsCancellationRequested) + { + throw new ChannelClosedException(); + } + } + + if (!_reader.TryRead(out var connection)) + { + throw new ChannelClosedException(); + } + + if (connection is not null) + { + Interlocked.Decrement(ref _count); + } + + return connection; + } + + /// + /// Synchronously blocks the calling thread until an item is available. The wait is + /// released directly by the writing thread, so it never depends on a thread pool + /// continuation the way sync-over-async on does. + /// + internal DbConnectionInternal? Read(CancellationToken cancellationToken) + { + if (!_items.Wait(0)) + { + using var linked = CancellationTokenSource.CreateLinkedTokenSource( + cancellationToken, _completedCts.Token); + try + { + _items.Wait(linked.Token); + } + catch (OperationCanceledException) when (!cancellationToken.IsCancellationRequested) + { + throw new ChannelClosedException(); + } + } + + if (!_reader.TryRead(out var connection)) + { + throw new ChannelClosedException(); + } if (connection is not null) { diff --git a/src/Microsoft.Data.SqlClient/tests/PerformanceTests/BenchmarkRunners/ConnectionPoolRampRunner.cs b/src/Microsoft.Data.SqlClient/tests/PerformanceTests/BenchmarkRunners/ConnectionPoolRampRunner.cs new file mode 100644 index 0000000000..2c5d172c71 --- /dev/null +++ b/src/Microsoft.Data.SqlClient/tests/PerformanceTests/BenchmarkRunners/ConnectionPoolRampRunner.cs @@ -0,0 +1,150 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. +// See the LICENSE file in the project root for more information. + +using System; +using System.Threading; +using System.Threading.Tasks; +using BenchmarkDotNet.Attributes; + +namespace Microsoft.Data.SqlClient.PerformanceTests +{ + /// + /// Measures how quickly a cold pool can ramp up to N physical connections when N + /// callers arrive simultaneously and all of them need a connection at the same time. + /// + /// This is the workload the ChannelDbConnectionPool (V2) was designed for. The legacy + /// WaitHandleDbConnectionPool guards creation with a Semaphore(1, 1), so a cold + /// burst of N callers establishes physical connections one at a time: total latency is + /// roughly N x connect latency. V2 has no such gate, so the opens overlap and total + /// latency approaches a single connect. + /// + /// Contrast with , which also + /// starts from a cold pool but releases each connection immediately. Because nothing is + /// held, one physical connection can satisfy every caller in turn, so that benchmark + /// rewards a pool that grows as slowly as possible and penalizes concurrent creation. + /// Holding each connection until every caller has one removes that artifact: the pool + /// genuinely needs N connections, and the only variable left is how fast it can open + /// them. + /// + /// is always larger than so no + /// caller ever waits for a connection to be returned. Back-pressure on a saturated pool + /// is covered separately by . + /// + /// The pool implementation (legacy vs V2) is a process-level choice - see the remarks on + /// . Run twice (UseConnectionPoolV2 false then + /// true) to compare. + /// + /// Related issue: #3356 + /// + public class ConnectionPoolRampRunner : BaseRunner + { + /// + /// Number of callers that arrive simultaneously against a cold pool. Each one holds + /// its connection until all of them have connected, so the pool must open exactly + /// this many physical connections. + /// + [Params(10, 25, 50)] + public int Parallelism { get; set; } + + /// + /// Max pool size. Deliberately larger than every value so + /// the ramp is never bounded by pool capacity. + /// + [Params(100)] + public int MaxPoolSize { get; set; } + + private string _connectionString; + + [GlobalSetup] + public void Setup() + { + Console.WriteLine( + "[ConnectionPoolRampRunner] Pool implementation: " + + (s_config.UseConnectionPoolV2 + ? "ChannelDbConnectionPool (V2)" + : "WaitHandleDbConnectionPool (legacy)")); + + var builder = new SqlConnectionStringBuilder(s_config.ConnectionString) + { + Pooling = true, + MaxPoolSize = MaxPoolSize, + // No pre-warming: every iteration must establish its own connections. + MinPoolSize = 0, + ConnectTimeout = 60 + }; + _connectionString = builder.ConnectionString; + } + + [IterationSetup] + public void IterationSetup() + { + // Start every iteration from a cold pool so the measurement is the ramp itself. + SqlConnection.ClearAllPools(); + } + + [GlobalCleanup] + public void Cleanup() => SqlConnection.ClearAllPools(); + + /// + /// Async cold-start ramp. All callers open concurrently and hold until the last one + /// has connected. + /// + [Benchmark] + public async Task ColdStartRampAsync() + { + using var allConnected = new CountdownEvent(Parallelism); + var release = new TaskCompletionSource( + TaskCreationOptions.RunContinuationsAsynchronously); + + var tasks = new Task[Parallelism]; + for (int i = 0; i < Parallelism; i++) + { + tasks[i] = Task.Run(async () => + { + using var conn = new SqlConnection(_connectionString); + await conn.OpenAsync(); + + // Hold the connection until every caller has one, forcing the pool to + // grow to Parallelism physical connections. + if (allConnected.Signal()) + { + release.TrySetResult(true); + } + + await release.Task; + // Dispose returns the connection to the pool. + }); + } + + await Task.WhenAll(tasks); + } + + /// + /// Sync cold-start ramp. Uses dedicated threads rather than thread pool threads so + /// the measurement reflects pool ramp latency rather than thread pool injection + /// delay, which would otherwise dominate once the callers block. + /// + [Benchmark] + public void ColdStartRamp() + { + using var allConnected = new CountdownEvent(Parallelism); + + var tasks = new Task[Parallelism]; + for (int i = 0; i < Parallelism; i++) + { + tasks[i] = Task.Factory.StartNew(() => + { + using var conn = new SqlConnection(_connectionString); + conn.Open(); + + allConnected.Signal(); + allConnected.Wait(); + // Dispose returns the connection to the pool. + }, TaskCreationOptions.LongRunning); + } + + Task.WaitAll(tasks); + } + } +} diff --git a/src/Microsoft.Data.SqlClient/tests/PerformanceTests/Config/Config.cs b/src/Microsoft.Data.SqlClient/tests/PerformanceTests/Config/Config.cs index 2bbd1f1c23..fde4bf4c17 100644 --- a/src/Microsoft.Data.SqlClient/tests/PerformanceTests/Config/Config.cs +++ b/src/Microsoft.Data.SqlClient/tests/PerformanceTests/Config/Config.cs @@ -70,6 +70,7 @@ public class Benchmarks public RunnerJob ConnectionPoolStressRunnerConfig; public RunnerJob ConnectionPoolContentionRunnerConfig; public RunnerJob ConnectionPoolChurnRunnerConfig; + public RunnerJob ConnectionPoolRampRunnerConfig; } public class RunnerJob diff --git a/src/Microsoft.Data.SqlClient/tests/PerformanceTests/Program.cs b/src/Microsoft.Data.SqlClient/tests/PerformanceTests/Program.cs index bf42ebaf75..a00a80ca01 100644 --- a/src/Microsoft.Data.SqlClient/tests/PerformanceTests/Program.cs +++ b/src/Microsoft.Data.SqlClient/tests/PerformanceTests/Program.cs @@ -54,6 +54,7 @@ public BenchmarkUnit(string name, Func selector, Type run new BenchmarkUnit("ConnectionPoolStress", b => b.ConnectionPoolStressRunnerConfig, typeof(ConnectionPoolStressRunner)), new BenchmarkUnit("ConnectionPoolContention", b => b.ConnectionPoolContentionRunnerConfig, typeof(ConnectionPoolContentionRunner)), new BenchmarkUnit("ConnectionPoolChurn", b => b.ConnectionPoolChurnRunnerConfig, typeof(ConnectionPoolChurnRunner)), + new BenchmarkUnit("ConnectionPoolRamp", b => b.ConnectionPoolRampRunnerConfig, typeof(ConnectionPoolRampRunner)), }; /// diff --git a/src/Microsoft.Data.SqlClient/tests/PerformanceTests/runnerconfig.jsonc b/src/Microsoft.Data.SqlClient/tests/PerformanceTests/runnerconfig.jsonc index 5777db9652..ba0b820172 100644 --- a/src/Microsoft.Data.SqlClient/tests/PerformanceTests/runnerconfig.jsonc +++ b/src/Microsoft.Data.SqlClient/tests/PerformanceTests/runnerconfig.jsonc @@ -142,6 +142,14 @@ "InvocationCount": 1, "WarmupCount": 1, "RowCount": 0 + }, + "ConnectionPoolRampRunnerConfig": { + "Enabled": true, + "LaunchCount": 1, + "IterationCount": 15, + "InvocationCount": 1, + "WarmupCount": 1, + "RowCount": 0 } } } diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/IdleConnectionChannelTest.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/IdleConnectionChannelTest.cs index dacc266753..433768edf0 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/IdleConnectionChannelTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/IdleConnectionChannelTest.cs @@ -311,6 +311,98 @@ await Task.WhenAll( #endregion + #region Read (synchronous) + + [Fact] + public void Read_WithBufferedConnection_ReturnsInlineAndDecrementsCount() + { + var channel = new IdleConnectionChannel(); + var connection = new StubDbConnectionInternal(); + channel.TryWrite(connection); + + Assert.Same(connection, channel.Read(CancellationToken.None)); + Assert.Equal(0, channel.Count); + } + + [Fact] + public void Read_BlocksUntilWritten_AndIsReleasedWithoutThreadPoolThreads() + { + var channel = new IdleConnectionChannel(); + var connection = new StubDbConnectionInternal(); + + // Starve the thread pool. A sync waiter must still be released, because the + // writing thread signals it directly rather than through a continuation. + ThreadPool.GetMinThreads(out int minWorker, out int minIo); + ThreadPool.GetMaxThreads(out int maxWorker, out int maxIo); + var blocked = new ManualResetEventSlim(false); + try + { + Assert.True(ThreadPool.SetMinThreads(1, minIo)); + Assert.True(ThreadPool.SetMaxThreads(1, maxIo)); + + // Occupy the single available worker thread for the duration of the wait. + ThreadPool.QueueUserWorkItem(_ => blocked.Wait()); + + DbConnectionInternal? read = null; + var reader = new Thread(() => read = channel.Read(CancellationToken.None)) + { + IsBackground = true + }; + reader.Start(); + + channel.TryWrite(connection); + + Assert.True(reader.Join(TimeSpan.FromSeconds(30))); + Assert.Same(connection, read); + Assert.Equal(0, channel.Count); + } + finally + { + blocked.Set(); + ThreadPool.SetMinThreads(minWorker, minIo); + ThreadPool.SetMaxThreads(maxWorker, maxIo); + } + } + + [Fact] + public void Read_WhenCancelled_ThrowsOperationCanceled() + { + var channel = new IdleConnectionChannel(); + using var cts = new CancellationTokenSource(); + + var reader = Task.Run(() => channel.Read(cts.Token)); + cts.CancelAfter(TimeSpan.FromMilliseconds(50)); + + Assert.ThrowsAny(() => reader.GetAwaiter().GetResult()); + } + + [Fact] + public void Read_AfterCompleteAndDrain_ThrowsChannelClosedException() + { + var channel = new IdleConnectionChannel(); + channel.TryWrite(new StubDbConnectionInternal()); + channel.Complete(); + + // Buffered item is still readable. + Assert.NotNull(channel.Read(CancellationToken.None)); + + Assert.Throws(() => channel.Read(CancellationToken.None)); + } + + [Fact] + public void Read_BlockedWaiter_IsReleasedByComplete() + { + var channel = new IdleConnectionChannel(); + + var reader = Task.Run(() => channel.Read(CancellationToken.None)); + Thread.Sleep(50); + channel.Complete(); + + Assert.Throws(() => reader.GetAwaiter().GetResult()); + } + + #endregion + #region Helpers private class StubDbConnectionInternal : DbConnectionInternal From 3e7b8b2fc49fa396619291113b2a19c099127df1 Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Mon, 17 Aug 2026 11:34:15 -0700 Subject: [PATCH 08/34] Include ConnectionPoolRamp in the connection-churn port tuning note Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- eng/pipelines/perf/scripts/run-perf-tests.sh | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/eng/pipelines/perf/scripts/run-perf-tests.sh b/eng/pipelines/perf/scripts/run-perf-tests.sh index a370761744..db36ef7134 100755 --- a/eng/pipelines/perf/scripts/run-perf-tests.sh +++ b/eng/pipelines/perf/scripts/run-perf-tests.sh @@ -332,7 +332,8 @@ export MALLOC_MMAP_THRESHOLD_="${MALLOC_MMAP_THRESHOLD_:-134217728}" # 128 MiB export MALLOC_TRIM_THRESHOLD_="${MALLOC_TRIM_THRESHOLD_:--1}" # never trim # --- §2.9 Network tuning (best-effort; needs privilege, so it must never fail the run) ------------ -# Connection-churn benches (ConnectionPoolStress, ParallelAsyncConnection) exhaust ephemeral ports; +# Connection-churn benches (ConnectionPoolStress, ConnectionPoolRamp, ParallelAsyncConnection) +# exhaust ephemeral ports; # widen the range and allow TIME_WAIT reuse so socket setup latency stays stable. 'sudo -n' keeps # this non-interactive: on a VM without passwordless sudo it fails immediately instead of blocking # on a password prompt, then we fall back to a non-sudo sysctl (and finally give up quietly). From e62688a4c57517114b3f914c67221c0bc05d5003 Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Mon, 17 Aug 2026 11:43:44 -0700 Subject: [PATCH 09/34] Correct the FIFO fairness claim on ChannelDbConnectionPool The semaphore-gated wait serves waiters in roughly arrival order but does not guarantee it, so the docs should not promise first-come, first-served. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../ConnectionPool/ChannelDbConnectionPool.cs | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs index ca019119de..990f177566 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs @@ -34,8 +34,10 @@ namespace Microsoft.Data.SqlClient.ConnectionPool /// threads, unlike wait handles which can block managed threads and potentially cause thread pool starvation. /// /// - /// FIFO fairness: Channels guarantee first-come, first-served ordering for connection requests, - /// ensuring fair access to connections under high contention scenarios. + /// FIFO fairness: connection requests are served in roughly first-come, first-served + /// order. This is best-effort, not a guarantee: waiters queue on a and then + /// take an item from the channel, and neither step promises strict ordering across sync and async + /// callers. /// /// /// Reduced lock contention: The channel-based approach minimizes lock usage compared to @@ -1441,8 +1443,9 @@ private async Task GetInternalConnection( timeout); // If we're at max capacity and couldn't open a connection. Block on the idle channel with a - // timeout. Note that Channels guarantee fair FIFO behavior to callers of ReadAsync - // (first-come, first-served), which is crucial to us. + // timeout. Ordering is best-effort: waiters queue on the channel's semaphore + // and then take an item, so a strict first-come, first-served guarantee no + // longer holds. if (async) { connection ??= await _idleChannel.ReadAsync(cancellationToken).ConfigureAwait(false); From 281aaff77b288c72cb82d3ab7e2c8b31b65efd36 Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Mon, 17 Aug 2026 11:53:53 -0700 Subject: [PATCH 10/34] Revert the sync-over-async channel wait change Measurement does not support it. On a saturated pool (50 sync workers, MaxPoolSize 10, so 40 concurrent waiters against 5 permits), raising the static semaphore from 5 to 100 permits changes nothing: 302 ms vs 280 ms at 5 ms hold, 60 ms vs 59 ms at 1 ms hold. The permit is released as soon as a connection arrives, so it caps how many callers are blocked, not throughput. Replacing the wait with a semaphore-gated channel measured identically to the original in every regime tested (saturated and unsaturated, 0/1/5 ms hold, thread pool and dedicated threads). It bought nothing and cost the FIFO fairness the pool documents as a headline property, since SemaphoreSlim does not guarantee release ordering and acquisition became two-stage. The residual v2 sync penalty is a fixed per-handoff cost on the wait path, visible only when the pool is saturated: +67% to +167% with zero work per operation, falling to +5% to +9% once each operation does 1 ms of work, and zero when Parallelism <= MaxPoolSize. That is a different and much smaller problem than the one this change addressed. Keeps ConnectionPoolRampRunner, which is unrelated. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../ConnectionPool/ChannelDbConnectionPool.cs | 52 +++++++++-- .../ConnectionPool/IdleConnectionChannel.cs | 67 +------------- .../IdleConnectionChannelTest.cs | 92 ------------------- 3 files changed, 47 insertions(+), 164 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs index 990f177566..e5b543e60c 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs @@ -34,10 +34,8 @@ namespace Microsoft.Data.SqlClient.ConnectionPool /// threads, unlike wait handles which can block managed threads and potentially cause thread pool starvation. /// /// - /// FIFO fairness: connection requests are served in roughly first-come, first-served - /// order. This is best-effort, not a guarantee: waiters queue on a and then - /// take an item from the channel, and neither step promises strict ordering across sync and async - /// callers. + /// FIFO fairness: Channels guarantee first-come, first-served ordering for connection requests, + /// ensuring fair access to connections under high contention scenarios. /// /// /// Reduced lock contention: The channel-based approach minimizes lock usage compared to @@ -62,6 +60,12 @@ namespace Microsoft.Data.SqlClient.ConnectionPool internal sealed class ChannelDbConnectionPool : IDbConnectionPool, IDisposable { #region Fields + // Limits synchronous operations which depend on async operations on managed + // threads from blocking on all available threads, which would stop async tasks + // from being scheduled and cause deadlocks. Use ProcessorCount/2 as a balance + // between sync and async tasks. + private static SemaphoreSlim _syncOverAsyncSemaphore = new(Math.Max(1, Environment.ProcessorCount / 2)); + /// /// Tracks the number of instances of this class. Used to generate unique IDs for each instance. /// @@ -1443,16 +1447,15 @@ private async Task GetInternalConnection( timeout); // If we're at max capacity and couldn't open a connection. Block on the idle channel with a - // timeout. Ordering is best-effort: waiters queue on the channel's semaphore - // and then take an item, so a strict first-come, first-served guarantee no - // longer holds. + // timeout. Note that Channels guarantee fair FIFO behavior to callers of ReadAsync + // (first-come, first-served), which is crucial to us. if (async) { connection ??= await _idleChannel.ReadAsync(cancellationToken).ConfigureAwait(false); } else { - connection ??= _idleChannel.Read(cancellationToken); + connection ??= ReadChannelSyncOverAsync(cancellationToken); } } catch (OperationCanceledException) @@ -1476,6 +1479,39 @@ private async Task GetInternalConnection( return connection; } + /// + /// Performs a blocking synchronous read from the idle connection channel. + /// + /// Cancels the read operation. + /// The connection read from the channel. + private DbConnectionInternal? ReadChannelSyncOverAsync(CancellationToken cancellationToken) + { + // If there are no connections in the channel, then ReadAsync will block until one is available. + // Channels doesn't offer a sync API, so running ReadAsync synchronously on this thread may spawn + // additional new async work items in the managed thread pool if there are no items available in the + // channel. We need to ensure that we don't block all available managed threads with these child + // tasks or we could deadlock. Prefer to block the current user-owned thread, and limit throughput + // to the managed threadpool. + + _syncOverAsyncSemaphore.Wait(cancellationToken); + try + { + ConfiguredValueTaskAwaitable.ConfiguredValueTaskAwaiter awaiter = + _idleChannel.ReadAsync(cancellationToken).ConfigureAwait(false).GetAwaiter(); + using ManualResetEventSlim mres = new ManualResetEventSlim(false, 0); + + // Cancellation happens through the ReadAsync call, which will complete the task. + // Even a failed task will complete and set the ManualResetEventSlim. + awaiter.UnsafeOnCompleted(() => mres.Set()); + mres.Wait(CancellationToken.None); + return awaiter.GetResult(); + } + finally + { + _syncOverAsyncSemaphore.Release(); + } + } + /// /// Sets connection state and activates the connection for use. Should always be called after a connection is /// created or retrieved from the pool. diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/IdleConnectionChannel.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/IdleConnectionChannel.cs index 8d2d5b953f..348ad33d9e 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/IdleConnectionChannel.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/IdleConnectionChannel.cs @@ -1,7 +1,6 @@ // Licensed to the .NET Foundation under one or more agreements. // The .NET Foundation licenses this file to you under the MIT license. // See the LICENSE file in the project root for more information. -using System; using System.Threading; using System.Threading.Tasks; using System.Threading.Channels; @@ -22,8 +21,6 @@ internal sealed class IdleConnectionChannel private readonly ChannelReader _reader; private readonly ChannelWriter _writer; private volatile int _count; - private readonly SemaphoreSlim _items = new(0); - private readonly CancellationTokenSource _completedCts = new(); internal IdleConnectionChannel() { @@ -40,12 +37,7 @@ internal IdleConnectionChannel() /// /// if this call completed the channel; otherwise /// (channel was already completed). - internal bool Complete() - { - bool completed = _writer.TryComplete(); - _completedCts.Cancel(); - return completed; - } + internal bool Complete() => _writer.TryComplete(); /// /// The number of non-null connections currently in the channel. @@ -65,7 +57,6 @@ internal bool TryWrite(DbConnectionInternal? connection) { Interlocked.Increment(ref _count); } - _items.Release(); return true; } @@ -78,8 +69,7 @@ internal bool TryWrite(DbConnectionInternal? connection) /// internal bool TryRead(out DbConnectionInternal? connection) { - connection = null; - if (_items.Wait(0) && _reader.TryRead(out connection)) + if (_reader.TryRead(out connection)) { if (connection is not null) { @@ -98,58 +88,7 @@ internal bool TryRead(out DbConnectionInternal? connection) /// internal async ValueTask ReadAsync(CancellationToken cancellationToken) { - if (!_items.Wait(0)) - { - using var linkedAsync = CancellationTokenSource.CreateLinkedTokenSource( - cancellationToken, _completedCts.Token); - try - { - await _items.WaitAsync(linkedAsync.Token).ConfigureAwait(false); - } - catch (OperationCanceledException) when (!cancellationToken.IsCancellationRequested) - { - throw new ChannelClosedException(); - } - } - - if (!_reader.TryRead(out var connection)) - { - throw new ChannelClosedException(); - } - - if (connection is not null) - { - Interlocked.Decrement(ref _count); - } - - return connection; - } - - /// - /// Synchronously blocks the calling thread until an item is available. The wait is - /// released directly by the writing thread, so it never depends on a thread pool - /// continuation the way sync-over-async on does. - /// - internal DbConnectionInternal? Read(CancellationToken cancellationToken) - { - if (!_items.Wait(0)) - { - using var linked = CancellationTokenSource.CreateLinkedTokenSource( - cancellationToken, _completedCts.Token); - try - { - _items.Wait(linked.Token); - } - catch (OperationCanceledException) when (!cancellationToken.IsCancellationRequested) - { - throw new ChannelClosedException(); - } - } - - if (!_reader.TryRead(out var connection)) - { - throw new ChannelClosedException(); - } + var connection = await _reader.ReadAsync(cancellationToken).ConfigureAwait(false); if (connection is not null) { diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/IdleConnectionChannelTest.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/IdleConnectionChannelTest.cs index 433768edf0..dacc266753 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/IdleConnectionChannelTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/IdleConnectionChannelTest.cs @@ -311,98 +311,6 @@ await Task.WhenAll( #endregion - #region Read (synchronous) - - [Fact] - public void Read_WithBufferedConnection_ReturnsInlineAndDecrementsCount() - { - var channel = new IdleConnectionChannel(); - var connection = new StubDbConnectionInternal(); - channel.TryWrite(connection); - - Assert.Same(connection, channel.Read(CancellationToken.None)); - Assert.Equal(0, channel.Count); - } - - [Fact] - public void Read_BlocksUntilWritten_AndIsReleasedWithoutThreadPoolThreads() - { - var channel = new IdleConnectionChannel(); - var connection = new StubDbConnectionInternal(); - - // Starve the thread pool. A sync waiter must still be released, because the - // writing thread signals it directly rather than through a continuation. - ThreadPool.GetMinThreads(out int minWorker, out int minIo); - ThreadPool.GetMaxThreads(out int maxWorker, out int maxIo); - var blocked = new ManualResetEventSlim(false); - try - { - Assert.True(ThreadPool.SetMinThreads(1, minIo)); - Assert.True(ThreadPool.SetMaxThreads(1, maxIo)); - - // Occupy the single available worker thread for the duration of the wait. - ThreadPool.QueueUserWorkItem(_ => blocked.Wait()); - - DbConnectionInternal? read = null; - var reader = new Thread(() => read = channel.Read(CancellationToken.None)) - { - IsBackground = true - }; - reader.Start(); - - channel.TryWrite(connection); - - Assert.True(reader.Join(TimeSpan.FromSeconds(30))); - Assert.Same(connection, read); - Assert.Equal(0, channel.Count); - } - finally - { - blocked.Set(); - ThreadPool.SetMinThreads(minWorker, minIo); - ThreadPool.SetMaxThreads(maxWorker, maxIo); - } - } - - [Fact] - public void Read_WhenCancelled_ThrowsOperationCanceled() - { - var channel = new IdleConnectionChannel(); - using var cts = new CancellationTokenSource(); - - var reader = Task.Run(() => channel.Read(cts.Token)); - cts.CancelAfter(TimeSpan.FromMilliseconds(50)); - - Assert.ThrowsAny(() => reader.GetAwaiter().GetResult()); - } - - [Fact] - public void Read_AfterCompleteAndDrain_ThrowsChannelClosedException() - { - var channel = new IdleConnectionChannel(); - channel.TryWrite(new StubDbConnectionInternal()); - channel.Complete(); - - // Buffered item is still readable. - Assert.NotNull(channel.Read(CancellationToken.None)); - - Assert.Throws(() => channel.Read(CancellationToken.None)); - } - - [Fact] - public void Read_BlockedWaiter_IsReleasedByComplete() - { - var channel = new IdleConnectionChannel(); - - var reader = Task.Run(() => channel.Read(CancellationToken.None)); - Thread.Sleep(50); - channel.Complete(); - - Assert.Throws(() => reader.GetAwaiter().GetResult()); - } - - #endregion - #region Helpers private class StubDbConnectionInternal : DbConnectionInternal From f917a818e8165aaef22634bcfad8b6909d5d1e89 Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Mon, 17 Aug 2026 12:34:00 -0700 Subject: [PATCH 11/34] Add thread-source variants to the connection pool benchmarks A sync Open() that waits blocks its thread. On threadpool threads that competes with the threadpool itself, so a pool whose waiter wake-up needs a queued continuation stalls until thread injection runs. The existing benchmarks could not separate that from an intrinsic pool cost. Adds a dedicated-thread variant of SteadyStateOpenQueryClose alongside the threadpool one, and a ConnectionPoolThreadPoolPressureRunner that pins the threadpool floor above and below the worker count so the effect is reproducible rather than dependent on hill-climbing timing. Existing benchmarks and their params are unchanged, to keep trend continuity. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- eng/pipelines/perf/README.md | 18 ++ eng/pipelines/perf/scripts/run-perf-tests.sh | 3 +- .../ConnectionPoolContentionRunner.cs | 47 +++++ .../ConnectionPoolThreadPoolPressureRunner.cs | 166 ++++++++++++++++++ .../tests/PerformanceTests/Config/Config.cs | 1 + .../tests/PerformanceTests/Program.cs | 1 + .../tests/PerformanceTests/runnerconfig.jsonc | 10 ++ 7 files changed, 245 insertions(+), 1 deletion(-) create mode 100644 src/Microsoft.Data.SqlClient/tests/PerformanceTests/BenchmarkRunners/ConnectionPoolThreadPoolPressureRunner.cs diff --git a/eng/pipelines/perf/README.md b/eng/pipelines/perf/README.md index ccf6428d1d..4e4493e938 100644 --- a/eng/pipelines/perf/README.md +++ b/eng/pipelines/perf/README.md @@ -230,6 +230,24 @@ connected, so the pool genuinely needs N physical connections and the only varia it can open them. That rewards concurrent creation instead of penalising it, and it is the case `RapidFireOpenClose` cannot express. +The same principle applies to how a benchmark schedules its workers. A sync `Open()` that has to +wait blocks whichever thread it runs on, so a pool whose waiter wake-up needs a queued continuation +stalls when every threadpool thread is already blocked; the wake-up waits on thread injection, which +costs about a second each time. Threadpool threads are the realistic case, because sync database +calls in ASP.NET run on them, and they are the only configuration in which that stall is visible. +Benchmarks therefore keep threadpool threads as the default and add dedicated-thread variants +alongside rather than instead: + +- `ConnectionPoolContentionRunner.SteadyStateOpenQueryCloseDedicatedThreads` runs the existing + workload on dedicated threads. A regression in both variants points at the pool; a regression in + only the threadpool variant points at the waiter wake path. +- `ConnectionPoolThreadPoolPressureRunner` pins the threadpool floor via `[Params]`, below the worker + count (starved) and above it (control), so the effect is reproducible instead of depending on + hill-climbing timing. + +This failure mode is tail latency, not a shifted median, so compare distributions. Aggregating with +a per-configuration minimum hides it completely. + Without somewhere to record that, the same benchmarks get re-investigated every run and `--fail-on-regression` is unusable for experiments. Each switch may therefore have an annotation file at `expected-differences/.json`, picked up automatically by diff --git a/eng/pipelines/perf/scripts/run-perf-tests.sh b/eng/pipelines/perf/scripts/run-perf-tests.sh index db36ef7134..57e2deb2d8 100755 --- a/eng/pipelines/perf/scripts/run-perf-tests.sh +++ b/eng/pipelines/perf/scripts/run-perf-tests.sh @@ -332,7 +332,8 @@ export MALLOC_MMAP_THRESHOLD_="${MALLOC_MMAP_THRESHOLD_:-134217728}" # 128 MiB export MALLOC_TRIM_THRESHOLD_="${MALLOC_TRIM_THRESHOLD_:--1}" # never trim # --- §2.9 Network tuning (best-effort; needs privilege, so it must never fail the run) ------------ -# Connection-churn benches (ConnectionPoolStress, ConnectionPoolRamp, ParallelAsyncConnection) +# Connection-churn benches (ConnectionPoolStress, ConnectionPoolRamp, +# ConnectionPoolThreadPoolPressure, ParallelAsyncConnection) # exhaust ephemeral ports; # widen the range and allow TIME_WAIT reuse so socket setup latency stays stable. 'sudo -n' keeps # this non-interactive: on a VM without passwordless sudo it fails immediately instead of blocking diff --git a/src/Microsoft.Data.SqlClient/tests/PerformanceTests/BenchmarkRunners/ConnectionPoolContentionRunner.cs b/src/Microsoft.Data.SqlClient/tests/PerformanceTests/BenchmarkRunners/ConnectionPoolContentionRunner.cs index 899d25f64c..e99875bb92 100644 --- a/src/Microsoft.Data.SqlClient/tests/PerformanceTests/BenchmarkRunners/ConnectionPoolContentionRunner.cs +++ b/src/Microsoft.Data.SqlClient/tests/PerformanceTests/BenchmarkRunners/ConnectionPoolContentionRunner.cs @@ -30,6 +30,14 @@ namespace Microsoft.Data.SqlClient.PerformanceTests /// remarks on . Run twice (UseConnectionPoolV2 /// false then true) to compare. /// + /// Each sync workload is measured twice, once on threadpool threads + /// () and once on dedicated threads + /// (). Threadpool threads are the + /// realistic case, since sync database calls in ASP.NET run on them, and they are the + /// only configuration that can expose a waiter wake path which depends on the + /// threadpool having a free thread. Dedicated threads isolate the pool's own cost. The + /// pair separates a pool regression from a scheduling one. + /// /// Related issues: #601, #979, #3356 /// public class ConnectionPoolContentionRunner : BaseRunner @@ -114,6 +122,45 @@ public Task SteadyStateOpenQueryClose() return Task.WhenAll(tasks); } + /// + /// Same workload as , but driven by dedicated + /// threads instead of threadpool threads. + /// + /// Read the two together. A sync Open() that has to wait blocks whichever + /// thread it is running on. On threadpool threads that competes with the threadpool + /// itself, because a pool implementation whose waiter wake-up depends on a queued + /// continuation cannot make progress while every thread is blocked in a wait: the + /// wake-up is stuck behind thread injection, which adds roughly a second per stall. + /// Dedicated threads remove that coupling, so this variant measures the pool's + /// intrinsic checkout/return cost with the scheduler taken out of the picture. + /// + /// A regression in both points at the pool itself. A regression only in the + /// threadpool variant points at the waiter wake path and shows up as tail latency + /// rather than a shifted median, so compare the distribution and not just the mean. + /// + [Benchmark] + public Task SteadyStateOpenQueryCloseDedicatedThreads() + { + var tasks = new Task[Parallelism]; + for (int i = 0; i < Parallelism; i++) + { + tasks[i] = Task.Factory.StartNew(() => + { + for (int op = 0; op < OpsPerWorker; op++) + { + using var conn = new SqlConnection(_connectionString); + conn.Open(); + using var cmd = conn.CreateCommand(); + cmd.CommandText = "SELECT 1"; + _ = cmd.ExecuteScalar(); + // Dispose returns the connection to the pool. + } + }, TaskCreationOptions.LongRunning); + } + + return Task.WhenAll(tasks); + } + [Benchmark] public Task SteadyStateOpenQueryCloseAsync() { diff --git a/src/Microsoft.Data.SqlClient/tests/PerformanceTests/BenchmarkRunners/ConnectionPoolThreadPoolPressureRunner.cs b/src/Microsoft.Data.SqlClient/tests/PerformanceTests/BenchmarkRunners/ConnectionPoolThreadPoolPressureRunner.cs new file mode 100644 index 0000000000..da962e4f4f --- /dev/null +++ b/src/Microsoft.Data.SqlClient/tests/PerformanceTests/BenchmarkRunners/ConnectionPoolThreadPoolPressureRunner.cs @@ -0,0 +1,166 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. +// See the LICENSE file in the project root for more information. + +using System; +using System.Threading; +using System.Threading.Tasks; +using BenchmarkDotNet.Attributes; + +namespace Microsoft.Data.SqlClient.PerformanceTests +{ + /// + /// Measures a saturated pool driven by sync callers on threadpool threads, with the + /// threadpool's minimum worker count pinned so the result is reproducible. + /// + /// A sync Open() against a saturated pool blocks its thread. When those threads + /// are threadpool threads, a pool whose waiter wake-up requires a queued continuation + /// cannot make progress: every thread is blocked in a wait, so the wake-up sits in the + /// queue until the threadpool injects another thread. Injection is rate limited to + /// roughly one or two threads per second, so each stall costs about a second. + /// + /// covers the same shape at the default + /// threadpool floor, which makes it dependent on hill-climbing timing and therefore + /// noisy. Pinning the floor turns that into a controlled comparison: + /// + /// - below guarantees the + /// threadpool starts starved, so the wake path is exercised on every run. + /// - above pre-creates enough + /// threads that injection never gates progress. This is the control: a pool that only + /// regresses in the starved configuration has a wake-path problem, not a throughput + /// problem. + /// + /// The effect is tail latency, not a shifted median, so compare distributions rather + /// than means alone. + /// + /// The pool implementation (legacy vs V2) is a process-level choice - see the remarks on + /// . Run twice (UseConnectionPoolV2 false then + /// true) to compare. + /// + /// Related issue: #3356 + /// + public class ConnectionPoolThreadPoolPressureRunner : BaseRunner + { + /// + /// Number of concurrent sync workers, all running on threadpool threads. + /// + [Params(50)] + public int Parallelism { get; set; } + + /// + /// Max pool size. Deliberately smaller than so most + /// workers must block waiting for a connection to be returned. Without that + /// back-pressure nobody waits and the wake path is never exercised. + /// + [Params(10)] + public int MaxPoolSize { get; set; } + + /// + /// Threadpool minimum worker thread count, pinned for the duration of the run. The + /// low value is below (starved); the high value is above + /// it (control). + /// + [Params(8, 128)] + public int MinWorkerThreads { get; set; } + + /// + /// Number of open/query/close operations each worker performs per invocation. + /// + [Params(20)] + public int OpsPerWorker { get; set; } + + private string _connectionString; + private int _originalMinWorkerThreads; + private int _originalMinCompletionPortThreads; + + [GlobalSetup] + public void Setup() + { + Console.WriteLine( + "[ConnectionPoolThreadPoolPressureRunner] Pool implementation: " + + (s_config.UseConnectionPoolV2 + ? "ChannelDbConnectionPool (V2)" + : "WaitHandleDbConnectionPool (legacy)")); + + ThreadPool.GetMinThreads( + out _originalMinWorkerThreads, out _originalMinCompletionPortThreads); + ThreadPool.SetMinThreads(MinWorkerThreads, _originalMinCompletionPortThreads); + + var builder = new SqlConnectionStringBuilder(s_config.ConnectionString) + { + Pooling = true, + MaxPoolSize = MaxPoolSize, + MinPoolSize = 0 + }; + _connectionString = builder.ConnectionString; + } + + [GlobalCleanup] + public void Cleanup() + { + ThreadPool.SetMinThreads( + _originalMinWorkerThreads, _originalMinCompletionPortThreads); + } + + [IterationSetup] + public void IterationSetup() + { + // Warm the pool to MaxPoolSize so the measured run reflects steady-state + // checkout/return rather than first-time physical connection establishment. + WarmPool(MaxPoolSize); + } + + [IterationCleanup] + public void IterationCleanup() + { + using var conn = new SqlConnection(_connectionString); + SqlConnection.ClearPool(conn); + } + + [Benchmark] + public Task SaturatedSyncOpenOnThreadPool() + { + var tasks = new Task[Parallelism]; + for (int i = 0; i < Parallelism; i++) + { + tasks[i] = Task.Run(() => + { + for (int op = 0; op < OpsPerWorker; op++) + { + using var conn = new SqlConnection(_connectionString); + conn.Open(); + using var cmd = conn.CreateCommand(); + cmd.CommandText = "SELECT 1"; + _ = cmd.ExecuteScalar(); + // Dispose returns the connection to the pool. + } + }); + } + + return Task.WhenAll(tasks); + } + + private void WarmPool(int count) + { + var conns = new SqlConnection[count]; + try + { + for (int i = 0; i < count; i++) + { + conns[i] = new SqlConnection(_connectionString); + conns[i].Open(); + } + } + finally + { + // Close and dispose every connection so they return to the pool and + // are not retained until GC (which would add allocation/GC noise). + for (int i = 0; i < count; i++) + { + conns[i]?.Close(); + conns[i]?.Dispose(); + } + } + } + } +} diff --git a/src/Microsoft.Data.SqlClient/tests/PerformanceTests/Config/Config.cs b/src/Microsoft.Data.SqlClient/tests/PerformanceTests/Config/Config.cs index fde4bf4c17..6afb2067f2 100644 --- a/src/Microsoft.Data.SqlClient/tests/PerformanceTests/Config/Config.cs +++ b/src/Microsoft.Data.SqlClient/tests/PerformanceTests/Config/Config.cs @@ -71,6 +71,7 @@ public class Benchmarks public RunnerJob ConnectionPoolContentionRunnerConfig; public RunnerJob ConnectionPoolChurnRunnerConfig; public RunnerJob ConnectionPoolRampRunnerConfig; + public RunnerJob ConnectionPoolThreadPoolPressureRunnerConfig; } public class RunnerJob diff --git a/src/Microsoft.Data.SqlClient/tests/PerformanceTests/Program.cs b/src/Microsoft.Data.SqlClient/tests/PerformanceTests/Program.cs index a00a80ca01..8fd925cdf3 100644 --- a/src/Microsoft.Data.SqlClient/tests/PerformanceTests/Program.cs +++ b/src/Microsoft.Data.SqlClient/tests/PerformanceTests/Program.cs @@ -55,6 +55,7 @@ public BenchmarkUnit(string name, Func selector, Type run new BenchmarkUnit("ConnectionPoolContention", b => b.ConnectionPoolContentionRunnerConfig, typeof(ConnectionPoolContentionRunner)), new BenchmarkUnit("ConnectionPoolChurn", b => b.ConnectionPoolChurnRunnerConfig, typeof(ConnectionPoolChurnRunner)), new BenchmarkUnit("ConnectionPoolRamp", b => b.ConnectionPoolRampRunnerConfig, typeof(ConnectionPoolRampRunner)), + new BenchmarkUnit("ConnectionPoolThreadPoolPressure", b => b.ConnectionPoolThreadPoolPressureRunnerConfig, typeof(ConnectionPoolThreadPoolPressureRunner)), }; /// diff --git a/src/Microsoft.Data.SqlClient/tests/PerformanceTests/runnerconfig.jsonc b/src/Microsoft.Data.SqlClient/tests/PerformanceTests/runnerconfig.jsonc index ba0b820172..d9088af050 100644 --- a/src/Microsoft.Data.SqlClient/tests/PerformanceTests/runnerconfig.jsonc +++ b/src/Microsoft.Data.SqlClient/tests/PerformanceTests/runnerconfig.jsonc @@ -150,6 +150,16 @@ "InvocationCount": 1, "WarmupCount": 1, "RowCount": 0 + }, + // Measures tail latency from threadpool starvation, so it needs more iterations + // than the others: a stall shows up in the distribution, not in the median. + "ConnectionPoolThreadPoolPressureRunnerConfig": { + "Enabled": true, + "LaunchCount": 1, + "IterationCount": 25, + "InvocationCount": 1, + "WarmupCount": 1, + "RowCount": 0 } } } From b2b45e0cf7504e853e01ea12ee491b0897c79ca6 Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Mon, 17 Aug 2026 13:33:52 -0700 Subject: [PATCH 12/34] Annotate the saturated sync contention delta as expected SteadyStateOpenQueryClose runs 50 synchronous workers on thread pool threads against a pool of 10, so most of them block in Open() and the thread pool has no free worker left to run the waiter wake-up. That is thread pool saturation, not pool throughput: medians match within 2% and the whole delta is tail latency, which disappears when the thread pool floor is raised. Keeping parallelism below the thread pool's worker count is the application's responsibility, as is pre-warming the thread pool, so this is annotated rather than fixed. ConnectionPoolThreadPoolPressureRunner characterises the boundary. Enabling AllowSynchronousContinuations on the idle channel was considered and rejected: it would resume the waiter's retrieval loop on the returning thread, and that loop can reach OpenNewInternalConnection, which opens a physical connection synchronously. A caller's Close() could then pay for another caller's connect, TLS handshake and login. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- eng/pipelines/perf/README.md | 7 +++++++ .../perf/expected-differences/UseConnectionPoolV2.json | 6 ++++++ .../ConnectionPoolThreadPoolPressureRunner.cs | 6 ++++++ 3 files changed, 19 insertions(+) diff --git a/eng/pipelines/perf/README.md b/eng/pipelines/perf/README.md index 4e4493e938..6dd0b7bd29 100644 --- a/eng/pipelines/perf/README.md +++ b/eng/pipelines/perf/README.md @@ -248,6 +248,13 @@ alongside rather than instead: This failure mode is tail latency, not a shifted median, so compare distributions. Aggregating with a per-configuration minimum hides it completely. +Note what those benchmarks are for. Saturating the thread pool with blocked synchronous calls is an +application configuration problem, not a pool defect: an application should keep its parallelism +below the thread pool's worker count so newly queued work still runs promptly, and pre-warming the +thread pool is the application's responsibility rather than the driver's. These benchmarks exist to +characterise where that boundary sits and to catch it moving, so a delta here is annotated rather +than fixed. + Without somewhere to record that, the same benchmarks get re-investigated every run and `--fail-on-regression` is unusable for experiments. Each switch may therefore have an annotation file at `expected-differences/.json`, picked up automatically by diff --git a/eng/pipelines/perf/expected-differences/UseConnectionPoolV2.json b/eng/pipelines/perf/expected-differences/UseConnectionPoolV2.json index 4ad02fe02c..032fb3802c 100644 --- a/eng/pipelines/perf/expected-differences/UseConnectionPoolV2.json +++ b/eng/pipelines/perf/expected-differences/UseConnectionPoolV2.json @@ -4,5 +4,11 @@ "method": "RapidFireOpenClose", "params": "*", "reason": "ChannelDbConnectionPool opens physical connections concurrently by design; WaitHandleDbConnectionPool serialises growth behind a Semaphore(1, 1) and its WaitAny prefers a freed connection over a creation slot. This benchmark calls ClearAllPools() in [IterationCleanup] and holds connections for zero time, so every iteration measures a cold-start burst where the extra parallel opens have nothing to amortise against. Measured with a counting connection factory at Parallelism=20/MaxPoolSize=50: v1 creates 1 physical connection, v2 creates 9. Serialising v2 growth removes the regression entirely, which is the behaviour v2 exists to avoid. Steady-state churn already favours v2 (RapidOpenCloseSingleThreadAsync -15.5%, MultiCommandReuse -8% to -23%), and ConnectionPoolRampRunner measures the same cold burst with connections held, which is where concurrent creation is supposed to pay off." + }, + { + "benchmark": "ConnectionPoolContentionRunner", + "method": "SteadyStateOpenQueryClose", + "params": "*", + "reason": "Measures thread pool saturation, not pool throughput. The benchmark runs Parallelism=50 synchronous workers on thread pool threads against MaxPoolSize=10, so most workers block in Open() and the thread pool has no free worker left to run the waiter wake-up. ChannelDbConnectionPool wakes a synchronous waiter through a channel continuation, which must be queued (Channel.CreateUnbounded defaults AllowSynchronousContinuations to false), so the wake waits on thread pool injection at roughly 1-2 threads/sec. WaitHandleDbConnectionPool signals a Semaphore, which the OS delivers directly. The effect is entirely tail latency: at Parallelism=50/MaxPoolSize=10 the medians match within 2% (v1 25.9ms, v2 26.4ms) while the means diverge (v1 37.9ms, v2 89.7ms) and p95 diverges hard (v1 76.0ms, v2 802.1ms). Raising the thread pool floor removes it: SetMinThreads(128) takes the delta from +171% to +3.5%. An application that blocks more thread pool threads than the pool has workers is already misconfigured, and warming the thread pool is the application's responsibility, not the driver's. Enabling AllowSynchronousContinuations was rejected: it would run the resumed waiter's loop on the returning thread, and that loop can reach OpenNewInternalConnection, a synchronous physical connect, so a caller's Close() could pay for another caller's TCP connect, TLS handshake and login. ConnectionPoolThreadPoolPressureRunner pins the thread pool floor above and below the worker count to characterise this boundary directly." } ] diff --git a/src/Microsoft.Data.SqlClient/tests/PerformanceTests/BenchmarkRunners/ConnectionPoolThreadPoolPressureRunner.cs b/src/Microsoft.Data.SqlClient/tests/PerformanceTests/BenchmarkRunners/ConnectionPoolThreadPoolPressureRunner.cs index da962e4f4f..2dd12490fd 100644 --- a/src/Microsoft.Data.SqlClient/tests/PerformanceTests/BenchmarkRunners/ConnectionPoolThreadPoolPressureRunner.cs +++ b/src/Microsoft.Data.SqlClient/tests/PerformanceTests/BenchmarkRunners/ConnectionPoolThreadPoolPressureRunner.cs @@ -33,6 +33,12 @@ namespace Microsoft.Data.SqlClient.PerformanceTests /// The effect is tail latency, not a shifted median, so compare distributions rather /// than means alone. /// + /// A regression here is a statement about application configuration, not a pool defect. + /// An application that blocks more thread pool threads than the thread pool has workers + /// is already misconfigured, and pre-warming the thread pool is the application's + /// responsibility rather than the driver's. This runner exists to characterise where + /// that boundary is and to catch it moving, not to drive the delta to zero. + /// /// The pool implementation (legacy vs V2) is a process-level choice - see the remarks on /// . Run twice (UseConnectionPoolV2 false then /// true) to compare. From 28f3c180a759519d2bc5bd70bec9ef0220bb89e3 Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Mon, 17 Aug 2026 13:44:49 -0700 Subject: [PATCH 13/34] Attribute the v2 pool allocation delta to connection count Measured against the in-process TDS server. A physical connection costs ~57 KB and both pools allocate the same per connection (v1 57,275 B, v2 57,701 B). Across all six RapidFireOpenClose parameter sets, the allocation delta divided by the extra Login7 count lands at 55.6-56.2 KB, so connection count accounts for the whole delta with nothing left for per-operation overhead. On a warm pool where neither implementation opens anything, v2's checkout and return path allocates 24-48 B/op more than v1 (+0.2% to +0.5%). Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../perf/expected-differences/UseConnectionPoolV2.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/eng/pipelines/perf/expected-differences/UseConnectionPoolV2.json b/eng/pipelines/perf/expected-differences/UseConnectionPoolV2.json index 032fb3802c..9d0484ca10 100644 --- a/eng/pipelines/perf/expected-differences/UseConnectionPoolV2.json +++ b/eng/pipelines/perf/expected-differences/UseConnectionPoolV2.json @@ -3,7 +3,7 @@ "benchmark": "ConnectionPoolStressRunner", "method": "RapidFireOpenClose", "params": "*", - "reason": "ChannelDbConnectionPool opens physical connections concurrently by design; WaitHandleDbConnectionPool serialises growth behind a Semaphore(1, 1) and its WaitAny prefers a freed connection over a creation slot. This benchmark calls ClearAllPools() in [IterationCleanup] and holds connections for zero time, so every iteration measures a cold-start burst where the extra parallel opens have nothing to amortise against. Measured with a counting connection factory at Parallelism=20/MaxPoolSize=50: v1 creates 1 physical connection, v2 creates 9. Serialising v2 growth removes the regression entirely, which is the behaviour v2 exists to avoid. Steady-state churn already favours v2 (RapidOpenCloseSingleThreadAsync -15.5%, MultiCommandReuse -8% to -23%), and ConnectionPoolRampRunner measures the same cold burst with connections held, which is where concurrent creation is supposed to pay off." + "reason": "ChannelDbConnectionPool opens physical connections concurrently by design; WaitHandleDbConnectionPool serialises growth behind a Semaphore(1, 1) and its WaitAny prefers a freed connection over a creation slot. This benchmark calls ClearAllPools() in [IterationCleanup] and holds connections for zero time, so every iteration measures a cold-start burst where the extra parallel opens have nothing to amortise against. Measured with a counting connection factory at Parallelism=20/MaxPoolSize=50: v1 creates 1 physical connection, v2 creates 9. Serialising v2 growth removes the regression entirely, which is the behaviour v2 exists to avoid. Steady-state churn already favours v2 (RapidOpenCloseSingleThreadAsync -15.5%, MultiCommandReuse -8% to -23%), and ConnectionPoolRampRunner measures the same cold burst with connections held, which is where concurrent creation is supposed to pay off. The allocation delta has the same single cause. A physical connection costs ~57 KB (measured directly against the in-process TDS server: v1 57,275 B, v2 57,701 B, i.e. the pools allocate the same per connection). Across all six parameter sets the entire allocation delta divided by the extra Login7 count lands at 55.6-56.2 KB, so it is accounted for by connection count alone with nothing left over for per-operation overhead. On a warm pool where neither implementation opens anything, the v2 checkout/return path allocates only 24-48 B/op more than v1 (+0.2% to +0.5%), which is the ManualResetEventSlim, closure and awaiter box a contended synchronous wait allocates." }, { "benchmark": "ConnectionPoolContentionRunner", From a0f55bb20871c916197fbe1fa6d0be2c7272e233 Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Mon, 17 Aug 2026 16:41:17 -0700 Subject: [PATCH 14/34] Remove expected-differences annotation machinery from perf pipeline Switch experiments surface intended behaviour changes as regressions. The annotation file was one way to record that verdict; explaining it in review is another, and it avoids a mute mechanism that has to be trusted. Removes the per-switch JSON rules, their loader and matcher, the expected-difference status and report section, and the --expected-differences plumbing through both run-perf-tests wrappers. The README guidance on writing benchmarks that measure the intended behaviour directly (ConnectionPoolRampRunner, the dedicated-thread and threadpool-pressure variants) is kept, since it stands on its own. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- eng/pipelines/perf/README.md | 44 +---- .../UseConnectionPoolV2.json | 14 -- eng/pipelines/perf/scripts/interleave_perf.py | 156 +----------------- eng/pipelines/perf/scripts/run-perf-tests.ps1 | 12 -- eng/pipelines/perf/scripts/run-perf-tests.sh | 11 -- 5 files changed, 13 insertions(+), 224 deletions(-) delete mode 100644 eng/pipelines/perf/expected-differences/UseConnectionPoolV2.json diff --git a/eng/pipelines/perf/README.md b/eng/pipelines/perf/README.md index 6dd0b7bd29..8ec3706a45 100644 --- a/eng/pipelines/perf/README.md +++ b/eng/pipelines/perf/README.md @@ -15,8 +15,7 @@ database. | `sqlclient-perf-experiment.yml` | Experiment pipeline. Same template, same scripts; both passes build the **same source** and differ only in one runner-config switch; **no Kusto ingestion**. | | `scripts/run-perf-tests.sh` | Linux on-VM entry point: install SDK, create DB, run benchmarks (interleaved or sequential), compare. Baseline is a released package (`--baseline-version`), another git ref's source (`--baseline-source-ref`), or the same source with one runner-config switch flipped off (`--switch-under-test`). | | `scripts/run-perf-tests.ps1` | Windows equivalent (ProcessorAffinity instead of `taskset`). | -| `scripts/interleave_perf.py` | Interleaved + best-of-N orchestrator: runs each unit baseline↔candidate back-to-back and confirms regressions across N passes. Also applies the expected-difference annotations. | -| `expected-differences/.json` | Per-switch annotations naming benchmarks whose regression is an intended consequence of that switch, each with a required reason. Picked up automatically by `--switch-under-test`. | +| `scripts/interleave_perf.py` | Interleaved + best-of-N orchestrator: runs each unit baseline↔candidate back-to-back and confirms regressions across N passes. | | `scripts/compare_perf.py` | Compares baseline vs current BenchmarkDotNet JSON → delta (md + json). Reused by the orchestrator. | | `scripts/perf_to_kusto.py` | Translates BenchmarkDotNet "full" JSON → Kusto `PerfRun` + `PerfBenchmarkResult` NDJSON. | | `scripts/ingest_kusto.py` | Queued Kusto ingestion (az CLI auth, runs on the agent). | @@ -213,7 +212,7 @@ experiment would corrupt the perf database three ways: The comparison report and the raw BenchmarkDotNet artifacts are published as usual, and the build is tagged `Switch ` so experiments are identifiable in the ADO build list. -### Expected differences +### Intended differences A switch experiment flips intended behaviour, so any benchmark that measures that behaviour regresses by design. The `UseConnectionPoolV2` case is the motivating example: `ChannelDbConnectionPool` opens @@ -223,8 +222,8 @@ physical connections concurrently, where `WaitHandleDbConnectionPool` serialises the extra parallel opens have nothing to amortise against. That regression is the trade-off working as intended, not a defect. -An annotation is the fallback, not the first answer. Where a benchmark can be written so it measures -the intended behaviour directly, prefer that. `ConnectionPoolRampRunner` was added for exactly this +Where a benchmark can be written so it measures the intended behaviour directly, prefer that over +explaining the result away in a review. `ConnectionPoolRampRunner` was added for exactly this reason: it keeps the cold pool but makes every caller *hold* its connection until all of them have connected, so the pool genuinely needs N physical connections and the only variable left is how fast it can open them. That rewards concurrent creation instead of penalising it, and it is the case @@ -252,39 +251,8 @@ Note what those benchmarks are for. Saturating the thread pool with blocked sync application configuration problem, not a pool defect: an application should keep its parallelism below the thread pool's worker count so newly queued work still runs promptly, and pre-warming the thread pool is the application's responsibility rather than the driver's. These benchmarks exist to -characterise where that boundary sits and to catch it moving, so a delta here is annotated rather -than fixed. - -Without somewhere to record that, the same benchmarks get re-investigated every run and -`--fail-on-regression` is unusable for experiments. Each switch may therefore have an annotation file -at `expected-differences/.json`, picked up automatically by -`--switch-under-test` / `-SwitchUnderTest` when present: - -```json -[ - { - "benchmark": "ConnectionPoolStressRunner", - "method": "RapidFireOpenClose", - "params": "*", - "reason": "Why this difference is intended, with the evidence behind that conclusion." - } -] -``` - -`benchmark`, `method` and `params` are shell-style globs, each defaulting to `*`. `reason` is -required and the file is rejected without it, so an annotation is always reviewable rather than a -silent mute. - -Matching entries are reported as `🔵 expected difference`, listed in their own section of -`comparison.md` under the reason, excluded from the confirmed-regression count and ignored by the -regression gate. Two deliberate limits keep this honest: - -* Only regressions are reclassified. An annotated benchmark that comes back **unchanged or improved** - keeps its real status, so an annotation can never hide a result getting better. -* The best-of-N confirmation count is still computed first, so an expected difference that stops - reproducing is still visible in the JSON. - -A switch with no annotation file simply reports every regression as a regression. +characterise where that boundary sits and to catch it moving, so a delta here is explained in review +rather than fixed. ## Two-pass build model diff --git a/eng/pipelines/perf/expected-differences/UseConnectionPoolV2.json b/eng/pipelines/perf/expected-differences/UseConnectionPoolV2.json deleted file mode 100644 index 9d0484ca10..0000000000 --- a/eng/pipelines/perf/expected-differences/UseConnectionPoolV2.json +++ /dev/null @@ -1,14 +0,0 @@ -[ - { - "benchmark": "ConnectionPoolStressRunner", - "method": "RapidFireOpenClose", - "params": "*", - "reason": "ChannelDbConnectionPool opens physical connections concurrently by design; WaitHandleDbConnectionPool serialises growth behind a Semaphore(1, 1) and its WaitAny prefers a freed connection over a creation slot. This benchmark calls ClearAllPools() in [IterationCleanup] and holds connections for zero time, so every iteration measures a cold-start burst where the extra parallel opens have nothing to amortise against. Measured with a counting connection factory at Parallelism=20/MaxPoolSize=50: v1 creates 1 physical connection, v2 creates 9. Serialising v2 growth removes the regression entirely, which is the behaviour v2 exists to avoid. Steady-state churn already favours v2 (RapidOpenCloseSingleThreadAsync -15.5%, MultiCommandReuse -8% to -23%), and ConnectionPoolRampRunner measures the same cold burst with connections held, which is where concurrent creation is supposed to pay off. The allocation delta has the same single cause. A physical connection costs ~57 KB (measured directly against the in-process TDS server: v1 57,275 B, v2 57,701 B, i.e. the pools allocate the same per connection). Across all six parameter sets the entire allocation delta divided by the extra Login7 count lands at 55.6-56.2 KB, so it is accounted for by connection count alone with nothing left over for per-operation overhead. On a warm pool where neither implementation opens anything, the v2 checkout/return path allocates only 24-48 B/op more than v1 (+0.2% to +0.5%), which is the ManualResetEventSlim, closure and awaiter box a contended synchronous wait allocates." - }, - { - "benchmark": "ConnectionPoolContentionRunner", - "method": "SteadyStateOpenQueryClose", - "params": "*", - "reason": "Measures thread pool saturation, not pool throughput. The benchmark runs Parallelism=50 synchronous workers on thread pool threads against MaxPoolSize=10, so most workers block in Open() and the thread pool has no free worker left to run the waiter wake-up. ChannelDbConnectionPool wakes a synchronous waiter through a channel continuation, which must be queued (Channel.CreateUnbounded defaults AllowSynchronousContinuations to false), so the wake waits on thread pool injection at roughly 1-2 threads/sec. WaitHandleDbConnectionPool signals a Semaphore, which the OS delivers directly. The effect is entirely tail latency: at Parallelism=50/MaxPoolSize=10 the medians match within 2% (v1 25.9ms, v2 26.4ms) while the means diverge (v1 37.9ms, v2 89.7ms) and p95 diverges hard (v1 76.0ms, v2 802.1ms). Raising the thread pool floor removes it: SetMinThreads(128) takes the delta from +171% to +3.5%. An application that blocks more thread pool threads than the pool has workers is already misconfigured, and warming the thread pool is the application's responsibility, not the driver's. Enabling AllowSynchronousContinuations was rejected: it would run the resumed waiter's loop on the returning thread, and that loop can reach OpenNewInternalConnection, a synchronous physical connect, so a caller's Close() could pay for another caller's TCP connect, TLS handshake and login. ConnectionPoolThreadPoolPressureRunner pins the thread pool floor above and below the worker count to characterise this boundary directly." - } -] diff --git a/eng/pipelines/perf/scripts/interleave_perf.py b/eng/pipelines/perf/scripts/interleave_perf.py index cd40acdff2..fdf66a5913 100644 --- a/eng/pipelines/perf/scripts/interleave_perf.py +++ b/eng/pipelines/perf/scripts/interleave_perf.py @@ -27,7 +27,6 @@ """ import argparse -import fnmatch import glob import json import os @@ -41,94 +40,6 @@ import compare_perf # noqa: E402 (local module, sits next to this script) -# -------------------------------------------------------------------------------------------------- -# Expected differences (switch experiments). -# -------------------------------------------------------------------------------------------------- -# A switch experiment compares the same build with one AppContext switch flipped, so any *intended* -# behavioural difference shows up as a regression. Example: UseConnectionPoolV2 deliberately allows -# concurrent physical connection opens, where the v1 pool serialises them behind a Semaphore(1, 1). -# Benchmarks that open connections in a burst against a cold pool therefore measure v2 creating more -# connections, which is the intended trade-off rather than a defect. -# -# Without a way to record that, every run re-litigates the same benchmarks and the --fail-on-regression -# gate is unusable for experiments. An expected-differences file names those benchmarks and, crucially, -# requires a reason, so the annotation is reviewable rather than a silent mute. - -def load_expected_differences(path): - """Loads an expected-differences file. - - The file is a JSON list of rules:: - - [ - { - "benchmark": "ConnectionPoolStressRunner", - "method": "RapidFireOpenClose", - "params": "*", - "reason": "v2 opens connections concurrently by design; ..." - } - ] - - ``benchmark``, ``method`` and ``params`` are shell-style globs and each defaults to ``*``. - ``reason`` is required: an unexplained entry is indistinguishable from hiding a real regression. - """ - with open(path, "r", encoding="utf-8") as handle: - raw = json.load(handle) - - if not isinstance(raw, list): - raise ValueError(f"{path}: expected a JSON list of rules, got {type(raw).__name__}.") - - rules = [] - for index, item in enumerate(raw): - if not isinstance(item, dict): - raise ValueError(f"{path}: rule {index} is not an object.") - reason = (item.get("reason") or "").strip() - if not reason: - raise ValueError( - f"{path}: rule {index} is missing a non-empty 'reason'. Every expected difference " - "must say why it is expected." - ) - rules.append({ - "benchmark": item.get("benchmark", "*"), - "method": item.get("method", "*"), - "params": item.get("params", "*"), - "reason": reason, - }) - return rules - - -def _rule_matches(rule, entry): - return ( - fnmatch.fnmatch(entry.get("benchmarkName") or "", rule["benchmark"]) - and fnmatch.fnmatch(entry.get("methodName") or "", rule["method"]) - and fnmatch.fnmatch(entry.get("parameterSignature") or "", rule["params"]) - ) - - -def apply_expected_differences(entries, rules): - """Reclassifies regressions that a rule marks as intended. - - Runs *after* the best-of-N verdict so the confirmation count is still recorded: an expected - difference that stops reproducing is itself worth noticing. Only regressions are reclassified; - an annotated benchmark that comes back unchanged or improved keeps its real status, so the - annotation cannot mask a result getting better. - """ - if not rules: - return [] - - annotated = [] - for entry in entries: - if entry["status"] not in ("regression", "regression-unconfirmed"): - continue - for rule in rules: - if _rule_matches(rule, entry): - entry["status"] = "expected-difference" - entry["confirmedRegression"] = False - entry["expectedReason"] = rule["reason"] - annotated.append(entry) - break - return annotated - - # -------------------------------------------------------------------------------------------------- # CPU affinity (client pinning, wiki §2.4) — best effort, cross platform. # -------------------------------------------------------------------------------------------------- @@ -323,7 +234,7 @@ def _regression_keys(baseline_dir, current_dir, threshold): return entries, {e["key"] for e in entries if e["status"] == "regression"} -def orchestrate(runner, units, results_dir, threshold, reps, expected_rules=None): +def orchestrate(runner, units, results_dir, threshold, reps): baseline_agg = os.path.join(results_dir, "baseline") current_agg = os.path.join(results_dir, "current") comparison_dir = os.path.join(results_dir, "comparison") @@ -379,12 +290,9 @@ def orchestrate(runner, units, results_dir, threshold, reps, expected_rules=None e["totalReps"] = total_reps e["confirmedRegression"] = False - # Applied after the verdict so an intended difference still carries its confirmation count. - expected = apply_expected_differences(entries, expected_rules) - confirmed = [e for e in entries if e.get("confirmedRegression")] unconfirmed = [e for e in entries if e["status"] == "regression-unconfirmed"] - return entries, confirmed, unconfirmed, expected + return entries, confirmed, unconfirmed # -------------------------------------------------------------------------------------------------- # Rendering. @@ -392,7 +300,6 @@ def orchestrate(runner, units, results_dir, threshold, reps, expected_rules=None _ICON = { "regression": "🔴 regression", "regression-unconfirmed": "🟠 regression (unconfirmed)", - "expected-difference": "🔵 expected difference", "improvement": "🟢 improvement", "unchanged": "⚪ unchanged", "current-only": "🆕 new", @@ -401,7 +308,7 @@ def orchestrate(runner, units, results_dir, threshold, reps, expected_rules=None } -def render_markdown(entries, confirmed, unconfirmed, expected, baseline_version, threshold, reps): +def render_markdown(entries, confirmed, unconfirmed, baseline_version, threshold, reps): improvements = [e for e in entries if e["status"] == "improvement"] lines = [] lines.append("# SqlClient Performance Comparison (interleaved, best-of-N)") @@ -414,9 +321,6 @@ def render_markdown(entries, confirmed, unconfirmed, expected, baseline_version, lines.append(f"- **Confirmed** regressions (majority of {reps}): **{len(confirmed)}**") lines.append(f"- Unconfirmed (flagged once, not reproduced): **{len(unconfirmed)}**") lines.append(f"- Improvements (faster > {threshold:.0f}%): **{len(improvements)}**") - if expected: - lines.append(f"- Expected differences (annotated, not counted as regressions): " - f"**{len(expected)}**") lines.append("") lines.append("| Status | Benchmark | Method | Params | Baseline (ms) | " "Current (ms) | Mean Δ | Alloc Δ | Confirm |") @@ -443,36 +347,6 @@ def render_markdown(entries, confirmed, unconfirmed, expected, baseline_version, ) lines.append("") - if expected: - lines.append("## Expected differences") - lines.append("") - lines.append("These benchmarks regressed, but the annotation file records the regression as an " - "intended consequence of the switch under test. They are excluded from the " - "confirmed-regression count and from the regression gate.") - lines.append("") - # Grouped by reason: one rule usually covers a whole family of parameterisations, and - # repeating a paragraph of rationale per row buries it. - by_reason = {} - for e in expected: - by_reason.setdefault(e.get("expectedReason", ""), []).append(e) - - for reason, group in by_reason.items(): - lines.append(reason) - lines.append("") - lines.append("| Benchmark | Method | Params | Mean Δ | Alloc Δ |") - lines.append("| --------- | ------ | ------ | ------ | ------- |") - for e in group: - lines.append( - "| {name} | {method} | {params} | {delta} | {alloc} |".format( - name=e["benchmarkName"], - method=e["methodName"], - params=e["parameterSignature"] or "-", - delta=compare_perf._fmt_pct(e["meanDeltaPct"]), - alloc=compare_perf._fmt_pct(e["allocDeltaPct"]), - ) - ) - lines.append("") - return "\n".join(lines) @@ -504,11 +378,6 @@ def main(argv=None): help="Override RUNNER_CONFIG for current-variant subprocesses only.") parser.add_argument("--client-cpus", default=os.environ.get("PERF_CLIENT_CPUS", ""), help="CPU set to pin the benchmark client to, e.g. '16-31'.") - parser.add_argument("--expected-differences", default=None, - help="Path to a JSON file naming benchmarks whose regression is an intended " - "consequence of the switch under test. Matching entries are reported " - "separately and do not trip --fail-on-regression. Each rule must carry " - "a reason.") parser.add_argument("--fail-on-regression", action="store_true", help="Exit non-zero if any CONFIRMED regression is detected.") args = parser.parse_args(argv) @@ -534,28 +403,18 @@ def main(argv=None): baseline_runner_config=args.baseline_runner_config, current_runner_config=args.current_runner_config) - expected_rules = [] - if args.expected_differences: - try: - expected_rules = load_expected_differences(args.expected_differences) - except (OSError, ValueError, json.JSONDecodeError) as exc: - print(f"ERROR: could not load --expected-differences: {exc}", file=sys.stderr) - return 1 - print(f"Loaded {len(expected_rules)} expected-difference rule(s) from " - f"{args.expected_differences}.") - units = runner.list_units() if not units: print("ERROR: no enabled benchmark units were reported by the current build.", file=sys.stderr) return 1 print(f"Enabled units ({len(units)}): {', '.join(units)}") - entries, confirmed, unconfirmed, expected = orchestrate( - runner, units, results_dir, args.threshold, args.reps, expected_rules) + entries, confirmed, unconfirmed = orchestrate( + runner, units, results_dir, args.threshold, args.reps) # Outputs. comparison_dir = os.path.join(results_dir, "comparison") - md = render_markdown(entries, confirmed, unconfirmed, expected, + md = render_markdown(entries, confirmed, unconfirmed, args.baseline_version, args.threshold, args.reps) with open(os.path.join(comparison_dir, "comparison.md"), "w", encoding="utf-8") as fh: fh.write(md + "\n") @@ -566,7 +425,6 @@ def main(argv=None): "confirmationRuns": args.reps, "confirmedRegressions": len(confirmed), "unconfirmedRegressions": len(unconfirmed), - "expectedDifferences": len(expected), "entries": entries, }, fh, indent=2) # Surface the comparison as the top-level run summary (collect-results attaches results/*.md). @@ -574,7 +432,7 @@ def main(argv=None): os.path.join(results_dir, "summary.md")) print(f"Interleaved comparison complete: {len(confirmed)} confirmed regression(s), " - f"{len(unconfirmed)} unconfirmed, {len(expected)} expected difference(s).") + f"{len(unconfirmed)} unconfirmed.") if args.fail_on_regression and confirmed: print("Confirmed regressions detected; failing as requested.", file=sys.stderr) diff --git a/eng/pipelines/perf/scripts/run-perf-tests.ps1 b/eng/pipelines/perf/scripts/run-perf-tests.ps1 index 0b14150be7..bf740e61eb 100644 --- a/eng/pipelines/perf/scripts/run-perf-tests.ps1 +++ b/eng/pipelines/perf/scripts/run-perf-tests.ps1 @@ -790,18 +790,6 @@ if ((-not [string]::IsNullOrEmpty($BaselineLabel)) -and ($RunMode -eq "interleav # flavour keeps sharing the single ambient RUNNER_CONFIG set above. if (-not [string]::IsNullOrEmpty($SwitchUnderTest)) { $interleaveArgs += @("--baseline-runner-config", $BaselineRunnerConfig, "--current-runner-config", $CurrentRunnerConfig) - - # A switch experiment flips intended behaviour, so benchmarks that measure that behaviour - # regress by design. If the switch has an annotation file, hand it to the comparison so those - # show up as expected differences instead of being re-litigated every run. Optional: a switch - # with no annotated benchmarks simply has no file. - $ExpectedDifferencesFile = Join-Path (Split-Path -Parent $ScriptDir) (Join-Path "expected-differences" "$SwitchUnderTest.json") - if (Test-Path $ExpectedDifferencesFile) { - Write-Host "Using expected-differences annotations: $ExpectedDifferencesFile" - $interleaveArgs += @("--expected-differences", $ExpectedDifferencesFile) - } else { - Write-Host "No expected-differences file for $SwitchUnderTest; all regressions will be reported as such." - } } if ($FailOnRegression) { Write-Host "Regression gate ENABLED: a CONFIRMED candidate-slower regression (> $RegressionThreshold%) will fail the run." diff --git a/eng/pipelines/perf/scripts/run-perf-tests.sh b/eng/pipelines/perf/scripts/run-perf-tests.sh index 57e2deb2d8..6b1ab3a5c7 100755 --- a/eng/pipelines/perf/scripts/run-perf-tests.sh +++ b/eng/pipelines/perf/scripts/run-perf-tests.sh @@ -807,17 +807,6 @@ if [[ -n "${baselineLabel}" && "${runMode}" == "interleaved" ]]; then --baseline-runner-config "${BASELINE_RUNNER_CONFIG}" --current-runner-config "${CURRENT_RUNNER_CONFIG}" ) - # A switch experiment flips intended behaviour, so benchmarks that measure that behaviour - # regress by design. If the switch has an annotation file, hand it to the comparison so those - # show up as expected differences instead of being re-litigated every run. Optional: a switch - # with no annotated benchmarks simply has no file. - expectedDifferencesFile="${SCRIPT_DIR}/../expected-differences/${switchUnderTest}.json" - if [[ -f "${expectedDifferencesFile}" ]]; then - echo "Using expected-differences annotations: ${expectedDifferencesFile}" - interleave_args+=(--expected-differences "${expectedDifferencesFile}") - else - echo "No expected-differences file for ${switchUnderTest}; all regressions will be reported as such." - fi fi if [[ "${failOnRegression}" == "true" ]]; then echo "Regression gate ENABLED: a CONFIRMED candidate-slower regression (> ${regressionThreshold}%) will fail the run." From a065f01a3893e6eb7d00fc1b87a8ad2f498ed74d Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Mon, 17 Aug 2026 16:45:53 -0700 Subject: [PATCH 15/34] Reframe perf README switch-experiment section as benchmark design guidance The section still read as if 'intended differences' were a category the pipeline recognised. It no longer is. Retitles it, states plainly that there is no mute mechanism and why, and points at writing a benchmark that measures the intended behaviour instead. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- eng/pipelines/perf/README.md | 37 ++++++++++++++++++------------------ 1 file changed, 19 insertions(+), 18 deletions(-) diff --git a/eng/pipelines/perf/README.md b/eng/pipelines/perf/README.md index 8ec3706a45..42cef42d4a 100644 --- a/eng/pipelines/perf/README.md +++ b/eng/pipelines/perf/README.md @@ -212,22 +212,23 @@ experiment would corrupt the perf database three ways: The comparison report and the raw BenchmarkDotNet artifacts are published as usual, and the build is tagged `Switch ` so experiments are identifiable in the ADO build list. -### Intended differences - -A switch experiment flips intended behaviour, so any benchmark that measures that behaviour regresses -by design. The `UseConnectionPoolV2` case is the motivating example: `ChannelDbConnectionPool` opens -physical connections concurrently, where `WaitHandleDbConnectionPool` serialises growth behind a -`Semaphore(1, 1)`. `ConnectionPoolStressRunner.RapidFireOpenClose` calls `ClearAllPools()` in -`[IterationCleanup]` and holds connections for zero time, so it measures a cold-start burst in which -the extra parallel opens have nothing to amortise against. That regression is the trade-off working -as intended, not a defect. - -Where a benchmark can be written so it measures the intended behaviour directly, prefer that over -explaining the result away in a review. `ConnectionPoolRampRunner` was added for exactly this -reason: it keeps the cold pool but makes every caller *hold* its connection until all of them have -connected, so the pool genuinely needs N physical connections and the only variable left is how fast -it can open them. That rewards concurrent creation instead of penalising it, and it is the case -`RapidFireOpenClose` cannot express. +### Designing benchmarks for switch experiments + +A switch experiment flips behaviour on purpose, so a benchmark that measures that behaviour will +report a regression even when the change is working. `UseConnectionPoolV2` is the motivating example: +`ChannelDbConnectionPool` opens physical connections concurrently, where `WaitHandleDbConnectionPool` +serialises growth behind a `Semaphore(1, 1)`. `ConnectionPoolStressRunner.RapidFireOpenClose` calls +`ClearAllPools()` in `[IterationCleanup]` and holds connections for zero time, so it measures a +cold-start burst in which the extra parallel opens have nothing to amortise against. It reports the +trade-off as a loss because that is the only thing it can measure. + +The pipeline has no way to mark a result as acceptable, and deliberately so: a mute is only as good +as the reasoning behind it, and that reasoning belongs in the pull request where a reviewer can +challenge it. Prefer instead to add a benchmark that measures the intended behaviour directly. +`ConnectionPoolRampRunner` was added for exactly this reason: it keeps the cold pool but makes every +caller *hold* its connection until all of them have connected, so the pool genuinely needs N physical +connections and the only variable left is how fast it can open them. That rewards concurrent creation +instead of penalising it, and it is the case `RapidFireOpenClose` cannot express. The same principle applies to how a benchmark schedules its workers. A sync `Open()` that has to wait blocks whichever thread it runs on, so a pool whose waiter wake-up needs a queued continuation @@ -251,8 +252,8 @@ Note what those benchmarks are for. Saturating the thread pool with blocked sync application configuration problem, not a pool defect: an application should keep its parallelism below the thread pool's worker count so newly queued work still runs promptly, and pre-warming the thread pool is the application's responsibility rather than the driver's. These benchmarks exist to -characterise where that boundary sits and to catch it moving, so a delta here is explained in review -rather than fixed. +characterise where that boundary sits and to catch it moving, so a delta here is a prompt to check +the boundary has not shifted rather than a bug to fix. ## Two-pass build model From 4422d2ae8dbcdc9b07a9e93228add8040e4e8b69 Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Mon, 24 Aug 2026 14:32:58 -0700 Subject: [PATCH 16/34] Fix hang in ConnectionPoolRampRunner when a worker fails to open Both ramp benchmarks used a rendezvous that was only reached on the success path. If any caller threw before signalling, the countdown never reached zero: the async path awaited a release TCS that was never completed, and the sync path blocked in CountdownEvent.Wait() with no timeout. Either one hangs the perf job indefinitely. Signal from a finally so a failed open still counts as arrived, and bound both waits so a barrier bug fails the run quickly instead of stalling it. The iteration now faults with the original open exception rather than hanging. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../ConnectionPoolRampRunner.cs | 47 +++++++++++++++---- 1 file changed, 37 insertions(+), 10 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/tests/PerformanceTests/BenchmarkRunners/ConnectionPoolRampRunner.cs b/src/Microsoft.Data.SqlClient/tests/PerformanceTests/BenchmarkRunners/ConnectionPoolRampRunner.cs index 2c5d172c71..aa43a47b99 100644 --- a/src/Microsoft.Data.SqlClient/tests/PerformanceTests/BenchmarkRunners/ConnectionPoolRampRunner.cs +++ b/src/Microsoft.Data.SqlClient/tests/PerformanceTests/BenchmarkRunners/ConnectionPoolRampRunner.cs @@ -56,6 +56,13 @@ public class ConnectionPoolRampRunner : BaseRunner private string _connectionString; + /// + /// Upper bound on the rendezvous wait. The rendezvous is released on the failure path + /// too, so this should never be reached; it exists so that a bug in the barrier fails + /// the run quickly instead of hanging the perf pipeline indefinitely. + /// + private static readonly TimeSpan s_rampTimeout = TimeSpan.FromMinutes(2); + [GlobalSetup] public void Setup() { @@ -103,16 +110,24 @@ public async Task ColdStartRampAsync() tasks[i] = Task.Run(async () => { using var conn = new SqlConnection(_connectionString); - await conn.OpenAsync(); - - // Hold the connection until every caller has one, forcing the pool to - // grow to Parallelism physical connections. - if (allConnected.Signal()) + try + { + await conn.OpenAsync(); + } + finally { - release.TrySetResult(true); + // Signal from a finally so a caller that fails to open still counts as + // arrived. Otherwise the countdown never reaches zero, the release is + // never set, and every other caller awaits forever. + if (allConnected.Signal()) + { + release.TrySetResult(true); + } } - await release.Task; + // Hold the connection until every caller has one, forcing the pool to + // grow to Parallelism physical connections. + await release.Task.WaitAsync(s_rampTimeout); // Dispose returns the connection to the pool. }); } @@ -136,10 +151,22 @@ public void ColdStartRamp() tasks[i] = Task.Factory.StartNew(() => { using var conn = new SqlConnection(_connectionString); - conn.Open(); + try + { + conn.Open(); + } + finally + { + // See ColdStartRampAsync: signalling from a finally keeps a failed + // open from stranding every other caller in Wait(). + allConnected.Signal(); + } - allConnected.Signal(); - allConnected.Wait(); + if (!allConnected.Wait(s_rampTimeout)) + { + throw new TimeoutException( + $"Cold-start ramp did not reach {Parallelism} connections within {s_rampTimeout}."); + } // Dispose returns the connection to the pool. }, TaskCreationOptions.LongRunning); } From 0d4f9a936fa90a19033c4baccb6e516413ef9e9e Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Mon, 24 Aug 2026 14:44:48 -0700 Subject: [PATCH 17/34] Count soft connects on the inline pool checkout fast path TryGetPooledConnectionInline activated the connection without recording a soft connect, so every checkout satisfied by the fast path went uncounted while its matching return still emitted a soft disconnect. This surfaced as DbConnectionPoolInstrumentationTest reporting 152 soft connects instead of 400. Record the request immediately before activation, matching GetInternalConnection: counting first keeps the paired soft disconnect from the PrepareConnection failure path from driving the active gauge negative. Both call sites return as soon as the fast path succeeds, so the request cannot be counted twice. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs index ae4b2645c9..7a3404fa6a 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs @@ -1533,6 +1533,11 @@ private void RemoveConnection(DbConnectionInternal connection) return null; } + // Counted before activation for the same reason as GetInternalConnection: if + // PrepareConnection fails it returns the connection to the pool, which emits the + // matching soft disconnect. Counting after would leave that disconnect unpaired and + // drive the active-soft-connects gauge negative. + Metrics.SoftConnectRequest(); PrepareConnection(owningConnection, connection, transaction); return connection; } From 3f0e6e581f8bc4786060c4b79b35a97451e2b673 Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Mon, 24 Aug 2026 14:52:03 -0700 Subject: [PATCH 18/34] Fail loudly if the threadpool worker floor cannot be pinned ConnectionPoolThreadPoolPressureRunner ignored the result of SetMinThreads. MinWorkerThreads is the benchmark's only independent variable, so a refused or clamped request would leave both configurations running at the same floor and produce a comparison that looks valid while measuring nothing. Check the return value and read the floor back, throwing on either failure. Restore is guarded the same way, since the floor is process-wide and leaving it raised would change the conditions for every runner that follows. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../ConnectionPoolThreadPoolPressureRunner.cs | 35 +++++++++++++++++-- 1 file changed, 33 insertions(+), 2 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/tests/PerformanceTests/BenchmarkRunners/ConnectionPoolThreadPoolPressureRunner.cs b/src/Microsoft.Data.SqlClient/tests/PerformanceTests/BenchmarkRunners/ConnectionPoolThreadPoolPressureRunner.cs index 2dd12490fd..b3a158d2f7 100644 --- a/src/Microsoft.Data.SqlClient/tests/PerformanceTests/BenchmarkRunners/ConnectionPoolThreadPoolPressureRunner.cs +++ b/src/Microsoft.Data.SqlClient/tests/PerformanceTests/BenchmarkRunners/ConnectionPoolThreadPoolPressureRunner.cs @@ -90,7 +90,7 @@ public void Setup() ThreadPool.GetMinThreads( out _originalMinWorkerThreads, out _originalMinCompletionPortThreads); - ThreadPool.SetMinThreads(MinWorkerThreads, _originalMinCompletionPortThreads); + SetMinWorkerThreads(MinWorkerThreads, _originalMinCompletionPortThreads); var builder = new SqlConnectionStringBuilder(s_config.ConnectionString) { @@ -104,10 +104,41 @@ public void Setup() [GlobalCleanup] public void Cleanup() { - ThreadPool.SetMinThreads( + // Restoring matters beyond this benchmark: the floor is process-wide, so leaving it + // raised would silently change the conditions for every runner that follows in the + // same process. + SetMinWorkerThreads( _originalMinWorkerThreads, _originalMinCompletionPortThreads); } + /// + /// Pins the threadpool worker floor, failing loudly if it does not take effect. + /// + /// is the only variable this benchmark manipulates, so a + /// refused or clamped request would leave every configuration running at the same floor + /// and produce a comparison that looks valid but measures nothing. The return value alone + /// is not sufficient evidence, so the value is also read back. + /// + private static void SetMinWorkerThreads(int workerThreads, int completionPortThreads) + { + if (!ThreadPool.SetMinThreads(workerThreads, completionPortThreads)) + { + throw new InvalidOperationException( + $"ThreadPool.SetMinThreads({workerThreads}, {completionPortThreads}) was refused. " + + "The threadpool floor is this benchmark's independent variable, so the run would " + + "otherwise report a comparison that did not actually vary it."); + } + + ThreadPool.GetMinThreads(out int actualWorkerThreads, out _); + if (actualWorkerThreads != workerThreads) + { + throw new InvalidOperationException( + $"ThreadPool.SetMinThreads({workerThreads}, ...) reported success but the floor " + + $"read back as {actualWorkerThreads}. The runtime clamped the request, so this " + + "configuration would not measure the intended threadpool pressure."); + } + } + [IterationSetup] public void IterationSetup() { From 2486e477a742fc1ae3013d621b2b2c5c1306cba9 Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Mon, 24 Aug 2026 15:53:54 -0700 Subject: [PATCH 19/34] Document what RapidFireOpenClose actually measures The XML docs on this benchmark described behaviour it does not exercise, which makes its pool v1/v2 deltas easy to misread. - It claimed to measure "raw pool checkout/return throughput under contention", but every Parallelism value (10, 20, 25) is below every MaxPoolSize value (50, 100), so the pool never saturates. Only PoolExhaustionRecovery reaches the limit, and it oversubscribes explicitly. - MaxPoolSize claimed that "when Parallelism exceeds this, tasks must wait". That never happens here, and the V2 idle channel is unbounded, so a capacity that is never reached changes no behaviour. - It claimed the per-task loop count keeps total checkouts proportional to pool capacity. The Math.Max(20, ...) floor dominates in four of six combinations, so Parallelism 20 and 25 run identical workloads at both MaxPoolSize values. What it really measures is wake-up scheduling: with no work between checkout and return the tasks stay phase-locked, the idle channel sits near empty, the inline TryRead fast path misses, and most checkouts park in ReadAsync and resume on a threadpool continuation. A pooled checkout costs single-digit microseconds while this benchmark attributes tens of microseconds to each one. Docs now say that, and point readers at ConnectionPoolChurnRunner for per-checkout cost and ConnectionPoolContentionRunner for realistic concurrency. Comments only - no executable lines change, so previously published numbers remain directly comparable. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../ConnectionPoolStressRunner.cs | 44 +++++++++++++++++-- 1 file changed, 40 insertions(+), 4 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/tests/PerformanceTests/BenchmarkRunners/ConnectionPoolStressRunner.cs b/src/Microsoft.Data.SqlClient/tests/PerformanceTests/BenchmarkRunners/ConnectionPoolStressRunner.cs index 9dad6f41d0..b907c249bf 100644 --- a/src/Microsoft.Data.SqlClient/tests/PerformanceTests/BenchmarkRunners/ConnectionPoolStressRunner.cs +++ b/src/Microsoft.Data.SqlClient/tests/PerformanceTests/BenchmarkRunners/ConnectionPoolStressRunner.cs @@ -31,8 +31,16 @@ public class ConnectionPoolStressRunner : BaseRunner /// /// Max pool size — controls how many physical connections the pool can hold. - /// When Parallelism exceeds this, tasks must wait for a free connection. /// + /// + /// Note that every value is below every value here, so this + /// limit is only actually reached by , which + /// deliberately oversubscribes it. For the other benchmarks in this class the pool + /// never saturates and this parameter is close to inert: the V2 pool's idle channel is + /// unbounded, so a capacity that is never reached changes no behaviour. Treat a spread + /// between two MaxPoolSize values at the same Parallelism as a noise estimate rather + /// than a real effect. + /// [Params(50, 100)] public int MaxPoolSize { get; set; } @@ -79,13 +87,41 @@ public void Cleanup() /// /// Pure open/close churn — every task opens a pooled connection, immediately closes it, - /// and repeats. Measures raw pool checkout/return throughput under contention. - /// The per-task loop count scales with MaxPoolSize so total checkouts stay proportional - /// to pool capacity regardless of how the [Params] values change. + /// and repeats. /// + /// + /// + /// This is a scheduling benchmark, not a measure of raw checkout throughput. + /// Because nothing happens between checkout and return, the tasks stay phase-locked and + /// the idle channel oscillates around empty, so the inline TryRead fast path + /// misses and most checkouts park in ReadAsync and resume on a threadpool + /// continuation. What is measured is dominated by that wake-up cost plus + /// overhead: a pooled checkout costs single-digit + /// microseconds, while this benchmark attributes tens of microseconds to each one. + /// + /// + /// For the per-checkout cost of the pool's acquire/return path itself, use + /// , which runs this same loop single-threaded + /// against a warm pool. For concurrent behaviour under a realistic workload — where a + /// connection is held for the duration of a query, so returns and checkouts decorrelate + /// and the fast path hits — use . A + /// regression here alongside flat or improved results in those two indicates a change in + /// wake-up scheduling, not in checkout cost. + /// + /// + /// Also note that drops the pool between iterations, so + /// each iteration amortises a cold-start burst of physical connects (a few percent of + /// the mean). That largely cancels between baseline and current, but it adds variance. + /// + /// [Benchmark] public async Task RapidFireOpenClose() { + // NOTE: the Math.Max floor dominates for most [Params] combinations, so total + // checkouts do NOT scale cleanly with pool capacity — at Parallelism 20 and 25 both + // MaxPoolSize values run an identical workload. Kept as-is so results stay + // comparable with previously published runs; see the redesign issue before relying + // on the MaxPoolSize axis here. int iterationsPerTask = Math.Max(20, MaxPoolSize / Math.Max(1, Parallelism) * 4); var tasks = new Task[Parallelism]; for (int i = 0; i < Parallelism; i++) From ab213021131de3a4d1293b48b64d906652d3559a Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Mon, 24 Aug 2026 15:59:46 -0700 Subject: [PATCH 20/34] Pre-warm the pool in ConnectionPoolStressRunner This class exists to measure pool checkout and return, but it cleared the pool in [IterationCleanup], so every iteration re-established physical connections inside the measured body. Establishing a connection costs milliseconds while a pooled checkout costs microseconds, so creation dominated the measurement and the runner was largely reporting connection-establishment throughput. That also made it structurally unfair between the two pool implementations: v2 opens connections concurrently while v1 serialises growth behind a Semaphore(1, 1), so a benchmark built out of repeated cold starts scores the intended v2 behaviour as a loss. Now: - Min Pool Size is pinned equal to Max Pool Size. Both pools treat MinPoolSize as a hard floor, and v2 only arms its pruning timer when MinPoolSize < MaxPoolSize, so the warmed connections stay resident. - GlobalSetup calls PrewarmPool, which opens MaxPoolSize connections before releasing any of them. Holding them all is what forces the pool to create distinct connections; releasing as it goes would hand the same idle one back repeatedly and create only one. Min Pool Size alone is not sufficient either, since both pools backfill it on a background task, so the first measured iteration would still pay for creation - and would pay differently under each pool. - IterationCleanup no longer clears the pool. Table creation is scoped so its connection is returned before PrewarmPool runs, otherwise the warm-up would need MaxPoolSize + 1 live connections and block against the cap until Connect Timeout. Cold start remains covered deliberately by ConnectionPoolRampRunner, which holds every connection until all callers have connected, so the pool genuinely needs N connections. Docs and the perf README are updated to match. ParallelAsyncConnectionRunner is left alone on purpose: it reproduces #601/#979, where concurrent open storms against a cold pool are the actual subject, and it has Pooling=false variants. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- eng/pipelines/perf/README.md | 26 ++-- .../ConnectionPoolStressRunner.cs | 123 ++++++++++++------ 2 files changed, 98 insertions(+), 51 deletions(-) diff --git a/eng/pipelines/perf/README.md b/eng/pipelines/perf/README.md index 6459786056..faaeed0a23 100644 --- a/eng/pipelines/perf/README.md +++ b/eng/pipelines/perf/README.md @@ -217,18 +217,26 @@ tagged `Switch ` so experiments are identifiable in the ADO build list. A switch experiment flips behaviour on purpose, so a benchmark that measures that behaviour will report a regression even when the change is working. `UseConnectionPoolV2` is the motivating example: `ChannelDbConnectionPool` opens physical connections concurrently, where `WaitHandleDbConnectionPool` -serialises growth behind a `Semaphore(1, 1)`. `ConnectionPoolStressRunner.RapidFireOpenClose` calls -`ClearAllPools()` in `[IterationCleanup]` and holds connections for zero time, so it measures a -cold-start burst in which the extra parallel opens have nothing to amortise against. It reports the -trade-off as a loss because that is the only thing it can measure. +serialises growth behind a `Semaphore(1, 1)`. `ConnectionPoolStressRunner` used to call +`ClearAllPools()` in `[IterationCleanup]`, so every iteration was a cold-start burst in which the +extra parallel opens had nothing to amortise against, and the runner reported the trade-off as a loss +because that was the only thing it could measure. + +That is a benchmark defect rather than something to explain away, so the runner now pre-warms the +pool to full capacity in `[GlobalSetup]` and no longer clears it between iterations. Establishing a +connection costs milliseconds while a pooled checkout costs microseconds, so any creation left in the +measured body swamps the pool cost the runner exists to measure. Keep that separation in mind when +adding a pool benchmark: warm the pool first unless connection establishment is precisely the thing +under test. The pipeline has no way to mark a result as acceptable, and deliberately so: a mute is only as good as the reasoning behind it, and that reasoning belongs in the pull request where a reviewer can -challenge it. Prefer instead to add a benchmark that measures the intended behaviour directly. -`ConnectionPoolRampRunner` was added for exactly this reason: it keeps the cold pool but makes every -caller *hold* its connection until all of them have connected, so the pool genuinely needs N physical -connections and the only variable left is how fast it can open them. That rewards concurrent creation -instead of penalising it, and it is the case `RapidFireOpenClose` cannot express. +challenge it. Prefer instead to fix the benchmark, or add one that measures the intended behaviour +directly. `ConnectionPoolRampRunner` was added for exactly this reason: it keeps the cold pool but +makes every caller *hold* its connection until all of them have connected, so the pool genuinely needs +N physical connections and the only variable left is how fast it can open them. That rewards +concurrent creation instead of penalising it, and it isolates cold start now that the stress runner no +longer conflates it with checkout cost. The same principle applies to how a benchmark schedules its workers. A sync `Open()` that has to wait blocks whichever thread it runs on, so a pool whose waiter wake-up needs a queued continuation diff --git a/src/Microsoft.Data.SqlClient/tests/PerformanceTests/BenchmarkRunners/ConnectionPoolStressRunner.cs b/src/Microsoft.Data.SqlClient/tests/PerformanceTests/BenchmarkRunners/ConnectionPoolStressRunner.cs index b907c249bf..b771cc9f43 100644 --- a/src/Microsoft.Data.SqlClient/tests/PerformanceTests/BenchmarkRunners/ConnectionPoolStressRunner.cs +++ b/src/Microsoft.Data.SqlClient/tests/PerformanceTests/BenchmarkRunners/ConnectionPoolStressRunner.cs @@ -16,6 +16,14 @@ namespace Microsoft.Data.SqlClient.PerformanceTests /// - Connection reuse with interleaved queries /// - Pool exhaustion and recovery under pressure /// + /// The pool is pre-warmed to full capacity in and is deliberately + /// not cleared between iterations, so these benchmarks measure steady-state + /// checkout and return rather than physical connection establishment. Establishing + /// connections costs orders of magnitude more than a pooled checkout (milliseconds versus + /// microseconds), so leaving any creation in the measured body swamps the pool cost this + /// class exists to measure. Cold-start behaviour is covered separately and deliberately by + /// . + /// /// Related issues: #601, #979, #3356 /// public class ConnectionPoolStressRunner : BaseRunner @@ -33,13 +41,15 @@ public class ConnectionPoolStressRunner : BaseRunner /// Max pool size — controls how many physical connections the pool can hold. /// /// - /// Note that every value is below every value here, so this - /// limit is only actually reached by , which - /// deliberately oversubscribes it. For the other benchmarks in this class the pool - /// never saturates and this parameter is close to inert: the V2 pool's idle channel is - /// unbounded, so a capacity that is never reached changes no behaviour. Treat a spread - /// between two MaxPoolSize values at the same Parallelism as a noise estimate rather - /// than a real effect. + /// The pool is pre-warmed to this many connections and pinned there by an equal + /// Min Pool Size, so this is the number of connections actually resident for the + /// whole run rather than just a ceiling. Note that every value + /// is below every value here, so the limit itself is only reached by + /// , which deliberately oversubscribes it. For the + /// other benchmarks the pool never saturates, so this parameter mostly varies how many + /// idle connections the checkout path is choosing among. Treat a spread between two + /// MaxPoolSize values at the same Parallelism as a noise estimate rather than a real + /// effect. /// [Params(50, 100)] public int MaxPoolSize { get; set; } @@ -47,8 +57,11 @@ public class ConnectionPoolStressRunner : BaseRunner [GlobalSetup] public void Setup() { + // Pin Min Pool Size to Max Pool Size so the pool holds full capacity for the whole + // run: pruning cannot shrink it back down, and no benchmark body has to establish a + // physical connection. _connectionString = s_config.ConnectionString + - $";Pooling=True;Max Pool Size={MaxPoolSize};Min Pool Size=5;Connect Timeout=60"; + $";Pooling=True;Max Pool Size={MaxPoolSize};Min Pool Size={MaxPoolSize};Connect Timeout=60"; // Create a small table for query workloads. // Hash the machine name instead of using it verbatim: hostnames can be long enough @@ -56,22 +69,55 @@ public void Setup() // than using Math.Abs, which throws OverflowException when the hash is int.MinValue. string machineHash = ((uint)Environment.MachineName.GetHashCode()).ToString("x8"); _tableName = $"[perf_PoolStress_{machineHash}_{Guid.NewGuid():N}]"; - using var conn = new SqlConnection(_connectionString); - conn.Open(); - using var cmd = new SqlCommand( - $"CREATE TABLE {_tableName} (Id INT IDENTITY PRIMARY KEY, Val INT)", conn); - cmd.ExecuteNonQuery(); - // Seed a few rows so SELECT queries return data - using var insert = new SqlCommand( - $"INSERT INTO {_tableName} (Val) VALUES (1),(2),(3),(4),(5)", conn); - insert.ExecuteNonQuery(); + // Scoped so this connection is back in the pool before PrewarmPool runs: that method + // holds MaxPoolSize connections at once, so an extra live one would hit the cap and + // block until Connect Timeout. + { + using var conn = new SqlConnection(_connectionString); + conn.Open(); + using var cmd = new SqlCommand( + $"CREATE TABLE {_tableName} (Id INT IDENTITY PRIMARY KEY, Val INT)", conn); + cmd.ExecuteNonQuery(); + + // Seed a few rows so SELECT queries return data + using var insert = new SqlCommand( + $"INSERT INTO {_tableName} (Val) VALUES (1),(2),(3),(4),(5)", conn); + insert.ExecuteNonQuery(); + } + + PrewarmPool(); } - [IterationCleanup] - public void IterationCleanup() + /// + /// Fills the pool to live connections before any measurement + /// starts, so benchmark bodies only ever exercise checkout and return. + /// + /// + /// Every connection is opened before any is released. Releasing as we go would let the + /// pool hand the same idle connection back repeatedly and create only one, which is the + /// whole failure this is meant to avoid. Min Pool Size alone is not enough either: + /// it is backfilled lazily, so the first measured iteration would still pay for creation. + /// + private void PrewarmPool() { - SqlConnection.ClearAllPools(); + var warm = new SqlConnection[MaxPoolSize]; + try + { + for (int i = 0; i < warm.Length; i++) + { + warm[i] = new SqlConnection(_connectionString); + warm[i].Open(); + } + } + finally + { + // Returns them all to the pool; Min Pool Size keeps them resident from here on. + foreach (var conn in warm) + { + conn?.Dispose(); + } + } } [GlobalCleanup] @@ -87,31 +133,25 @@ public void Cleanup() /// /// Pure open/close churn — every task opens a pooled connection, immediately closes it, - /// and repeats. + /// and repeats. With the pool pre-warmed to full capacity, every open is a pure checkout, + /// so this measures the pool's acquire/return path under concurrency. /// /// /// - /// This is a scheduling benchmark, not a measure of raw checkout throughput. /// Because nothing happens between checkout and return, the tasks stay phase-locked and - /// the idle channel oscillates around empty, so the inline TryRead fast path - /// misses and most checkouts park in ReadAsync and resume on a threadpool - /// continuation. What is measured is dominated by that wake-up cost plus - /// overhead: a pooled checkout costs single-digit - /// microseconds, while this benchmark attributes tens of microseconds to each one. - /// - /// - /// For the per-checkout cost of the pool's acquire/return path itself, use - /// , which runs this same loop single-threaded - /// against a warm pool. For concurrent behaviour under a realistic workload — where a - /// connection is held for the duration of a query, so returns and checkouts decorrelate - /// and the fast path hits — use . A - /// regression here alongside flat or improved results in those two indicates a change in - /// wake-up scheduling, not in checkout cost. + /// the idle channel oscillates around empty even though the pool is full. That makes this + /// benchmark unusually sensitive to how a returned connection is handed to a waiting + /// caller: under the V2 pool an inline TryRead miss parks the caller in + /// ReadAsync, so it resumes on a threadpool continuation. Read it as a + /// zero-hold-time worst case for wake-up scheduling, not as typical application + /// behaviour: real callers do some work while holding a connection, which decorrelates + /// returns from checkouts and lets the fast path hit. /// /// - /// Also note that drops the pool between iterations, so - /// each iteration amortises a cold-start burst of physical connects (a few percent of - /// the mean). That largely cancels between baseline and current, but it adds variance. + /// Compare against for the same loop with no + /// concurrency, and for concurrency with a + /// realistic hold time. A regression here alongside flat or improved results in those two + /// indicates a change in wake-up scheduling rather than in checkout cost. /// /// [Benchmark] @@ -119,9 +159,8 @@ public async Task RapidFireOpenClose() { // NOTE: the Math.Max floor dominates for most [Params] combinations, so total // checkouts do NOT scale cleanly with pool capacity — at Parallelism 20 and 25 both - // MaxPoolSize values run an identical workload. Kept as-is so results stay - // comparable with previously published runs; see the redesign issue before relying - // on the MaxPoolSize axis here. + // MaxPoolSize values run an identical workload, which makes the spread between them + // a useful read on this benchmark's own noise floor. int iterationsPerTask = Math.Max(20, MaxPoolSize / Math.Max(1, Parallelism) * 4); var tasks = new Task[Parallelism]; for (int i = 0; i < Parallelism; i++) From 2ac2b2edfc23c22c89b4b50c112b4a19a6b3ea31 Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Mon, 24 Aug 2026 16:10:54 -0700 Subject: [PATCH 21/34] Add a sync variant of RapidFireOpenClose Pooled checkout coverage had a hole. RapidFireOpenClose is async-only, and the only concurrent sync coverage was ConnectionPoolContentionRunner, which holds a connection for a query against a saturated pool. Nothing measured concurrent sync checkout against a warm, non-saturating pool - which, after the pre-warm fix, is exactly the inline fast path this branch adds. The two paths diverge inside the pool, so this is worth measuring separately: the V2 pool has no synchronous channel read, so a sync caller that misses the inline fast path runs the async wait synchronously rather than awaiting it. Workers run on threadpool threads deliberately, matching the existing runners: sync database calls in ASP.NET run there. Also documents why ConnectionPoolChurnRunner is not made redundant by this. Its inner loop is identical, but it is single-threaded against a single pooled connection, so it carries no scheduling component and is far more sensitive: its sync and async variants agreed to within 0.3 pp on the last run, while RapidFireOpenClose's duplicate-workload parameter pairs disagreed by ~20 pp. Churn answers "did per-checkout cost or allocation move", the stress runner answers "did hand-off between threads move". A regression in one with a flat result in the other is the signal that separates them. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../ConnectionPoolChurnRunner.cs | 10 +++++ .../ConnectionPoolStressRunner.cs | 37 ++++++++++++++++++- 2 files changed, 45 insertions(+), 2 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/tests/PerformanceTests/BenchmarkRunners/ConnectionPoolChurnRunner.cs b/src/Microsoft.Data.SqlClient/tests/PerformanceTests/BenchmarkRunners/ConnectionPoolChurnRunner.cs index ca1930e451..9357dafd0c 100644 --- a/src/Microsoft.Data.SqlClient/tests/PerformanceTests/BenchmarkRunners/ConnectionPoolChurnRunner.cs +++ b/src/Microsoft.Data.SqlClient/tests/PerformanceTests/BenchmarkRunners/ConnectionPoolChurnRunner.cs @@ -20,6 +20,16 @@ namespace Microsoft.Data.SqlClient.PerformanceTests /// concern for the new ChannelDbConnectionPool, which aims to avoid extra allocations /// on the hot path (issue #3356). /// + /// This overlaps in shape — + /// the inner loop is identical — but not in purpose, and the two are not + /// interchangeable. Being single-threaded with exactly one pooled connection, this + /// runner has no scheduling or wake-up component, which makes it far more sensitive: + /// its sync and async variants have agreed to within 0.3 percentage points, whereas + /// the concurrent runner's own duplicate-workload parameter pairs have disagreed by + /// around 20. Use this one to decide whether per-checkout cost or allocation moved, and + /// the concurrent runner to decide whether hand-off between threads moved. A regression + /// there with a flat result here points at scheduling rather than at the checkout path. + /// /// The pool implementation (legacy vs V2) is a process-level choice — see the remarks /// on . Run twice (UseConnectionPoolV2 false /// then true) to compare. diff --git a/src/Microsoft.Data.SqlClient/tests/PerformanceTests/BenchmarkRunners/ConnectionPoolStressRunner.cs b/src/Microsoft.Data.SqlClient/tests/PerformanceTests/BenchmarkRunners/ConnectionPoolStressRunner.cs index b771cc9f43..40f211c9b8 100644 --- a/src/Microsoft.Data.SqlClient/tests/PerformanceTests/BenchmarkRunners/ConnectionPoolStressRunner.cs +++ b/src/Microsoft.Data.SqlClient/tests/PerformanceTests/BenchmarkRunners/ConnectionPoolStressRunner.cs @@ -10,7 +10,7 @@ namespace Microsoft.Data.SqlClient.PerformanceTests { /// /// Stress-tests the connection pool with randomized parallel access patterns: - /// - Massive concurrent open/close churn + /// - Massive concurrent open/close churn, both sync and async /// - Randomized hold durations simulating real workloads /// - Mixed sync/async callers competing for pooled connections /// - Connection reuse with interleaved queries @@ -179,7 +179,40 @@ public async Task RapidFireOpenClose() } /// - /// Randomized hold — each task opens a connection, holds it for a random duration + /// Sync counterpart to : the same zero-hold churn, but + /// every checkout goes through the blocking Open() path. + /// + /// + /// Worth measuring separately because the two paths diverge inside the pool. The V2 pool + /// has no synchronous channel read, so a sync caller that misses the inline fast path has + /// to run the async wait synchronously, which is a materially different cost from + /// awaiting it. With the pool pre-warmed and never saturated by these + /// values, this stays on the fast path and so measures + /// concurrent sync checkout without the wake-up behaviour that + /// covers under saturation. + /// + /// Workers run on threadpool threads deliberately: sync database calls in ASP.NET run + /// there, so that is the configuration whose behaviour actually matters. + /// + [Benchmark] + public async Task RapidFireOpenCloseSync() + { + int iterationsPerTask = Math.Max(20, MaxPoolSize / Math.Max(1, Parallelism) * 4); + var tasks = new Task[Parallelism]; + for (int i = 0; i < Parallelism; i++) + { + tasks[i] = Task.Run(() => + { + for (int j = 0; j < iterationsPerTask; j++) + { + using var conn = new SqlConnection(_connectionString); + conn.Open(); + // immediate return to pool + } + }); + } + await Task.WhenAll(tasks); + } /// (0-50ms), optionally runs a query, then returns it. Simulates realistic mixed /// workloads where some connections are held briefly and others longer. /// From 2104d10bd652536bd89156982e6034e65b3285a3 Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Mon, 24 Aug 2026 16:14:33 -0700 Subject: [PATCH 22/34] Add a pool-depth axis to ConnectionPoolChurnRunner The two pools order idle connections differently: the legacy pool pops from a ConcurrentStack (LIFO), the V2 pool reads from an unbounded Channel (FIFO). The legacy pool therefore keeps handing back the connection just released, while V2 cycles through every idle connection in turn. That difference was untestable. Churn held MinPoolSize=1, where both orderings return the only connection there is. ConnectionPoolStressRunner does reach depth 50-100, but its minimum Parallelism is 10, so those rows carry scheduling noise that swamps a per-checkout locality effect. Parameterise Churn on pool depth instead, and pre-warm to that depth so every measured open is a pure checkout. Opens all connections before releasing any, since releasing as we go would let the pool serve the same connection back and collapse every depth to 1. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../ConnectionPoolChurnRunner.cs | 78 +++++++++++++++---- 1 file changed, 64 insertions(+), 14 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/tests/PerformanceTests/BenchmarkRunners/ConnectionPoolChurnRunner.cs b/src/Microsoft.Data.SqlClient/tests/PerformanceTests/BenchmarkRunners/ConnectionPoolChurnRunner.cs index 9357dafd0c..27589752a8 100644 --- a/src/Microsoft.Data.SqlClient/tests/PerformanceTests/BenchmarkRunners/ConnectionPoolChurnRunner.cs +++ b/src/Microsoft.Data.SqlClient/tests/PerformanceTests/BenchmarkRunners/ConnectionPoolChurnRunner.cs @@ -22,13 +22,19 @@ namespace Microsoft.Data.SqlClient.PerformanceTests /// /// This overlaps in shape — /// the inner loop is identical — but not in purpose, and the two are not - /// interchangeable. Being single-threaded with exactly one pooled connection, this - /// runner has no scheduling or wake-up component, which makes it far more sensitive: - /// its sync and async variants have agreed to within 0.3 percentage points, whereas - /// the concurrent runner's own duplicate-workload parameter pairs have disagreed by - /// around 20. Use this one to decide whether per-checkout cost or allocation moved, and - /// the concurrent runner to decide whether hand-off between threads moved. A regression - /// there with a flat result here points at scheduling rather than at the checkout path. + /// interchangeable. Being single-threaded, this runner has no scheduling or wake-up + /// component, which makes it far more sensitive: its sync and async variants have + /// agreed to within 0.3 percentage points, whereas the concurrent runner's own + /// duplicate-workload parameter pairs have disagreed by around 20. Use this one to + /// decide whether per-checkout cost or allocation moved, and the concurrent runner to + /// decide whether hand-off between threads moved. A regression there with a flat result + /// here points at scheduling rather than at the checkout path. + /// + /// Adding a Parallelism=1 case to that runner would not replace this one. Its + /// [Params] are class-wide, so a single-threaded row would also be generated for + /// benchmarks it makes meaningless (MixedSyncAsyncContention would have no sync + /// worker left to mix in), and this runner's higher operation count and separate + /// iteration settings are what keep the signal clean. /// /// The pool implementation (legacy vs V2) is a process-level choice — see the remarks /// on . Run twice (UseConnectionPoolV2 false @@ -46,6 +52,22 @@ public class ConnectionPoolChurnRunner : BaseRunner [Params(1000)] public int OpsPerInvocation { get; set; } + /// + /// How many idle connections the pool holds while the single caller churns against it. + /// + /// + /// This is not a redundant axis, because the two pools order idle connections + /// differently: the legacy pool pops from a ConcurrentStack (LIFO) while the V2 + /// pool reads from an unbounded Channel (FIFO). At depth 1 that difference is + /// invisible, since both hand back the only connection there is. At a realistic depth + /// the legacy pool keeps returning the connection just released — one hot object — while + /// the V2 pool cycles through every idle connection in turn, touching all of their + /// buffers and parser state. Depth is therefore the axis that exposes reuse locality, + /// and measuring only depth 1 would hide it entirely. + /// + [Params(1, 100)] + public int PoolDepth { get; set; } + private string _connectionString; [GlobalSetup] @@ -61,16 +83,44 @@ public void Setup() { Pooling = true, MaxPoolSize = 100, - // Pre-warm a single connection so the very first checkout is served from - // the pool rather than establishing a physical connection. - MinPoolSize = 1 + // Pin the floor at the requested depth so pruning cannot shrink the pool back + // down mid-run and change what the benchmark is measuring. + MinPoolSize = PoolDepth }; _connectionString = builder.ConnectionString; - // Force the pool to exist and hold at least one idle connection. - using var warm = new SqlConnection(_connectionString); - warm.Open(); - warm.Close(); + PrewarmPool(); + } + + /// + /// Fills the pool with idle connections before measurement, so + /// every measured open is a pure checkout. + /// + /// + /// All connections are opened before any is released. Releasing as we go would let the + /// pool hand the same idle connection straight back and create only one, which would + /// silently collapse every depth to 1. MinPoolSize alone is not enough either: + /// both pools backfill it on a background task, so the first measured iteration would + /// otherwise still be racing that warm-up. + /// + private void PrewarmPool() + { + var warm = new SqlConnection[PoolDepth]; + try + { + for (int i = 0; i < warm.Length; i++) + { + warm[i] = new SqlConnection(_connectionString); + warm[i].Open(); + } + } + finally + { + foreach (var conn in warm) + { + conn?.Dispose(); + } + } } [GlobalCleanup] From 9ede8e06d79c034ee2a3522e04cfe24df98a2f1f Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Mon, 24 Aug 2026 16:18:41 -0700 Subject: [PATCH 23/34] Make Parallelism actually drive PoolExhaustionRecovery taskCount was Math.Max(Parallelism, MaxPoolSize * 2). MaxPoolSize * 2 is 100 or 200 and Parallelism never exceeds 25, so the parameter had no effect: the benchmark ran six configurations that were only two distinct workloads, with the other four exact duplicates. Use MaxPoolSize + Parallelism instead. Saturating the pool already takes MaxPoolSize tasks before anyone waits, so Parallelism is meaningful here as the oversubscription amount, which is the depth of the queue of waiting callers. All six configurations are now distinct and queue depth varies as intended. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../BenchmarkRunners/ConnectionPoolStressRunner.cs | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/tests/PerformanceTests/BenchmarkRunners/ConnectionPoolStressRunner.cs b/src/Microsoft.Data.SqlClient/tests/PerformanceTests/BenchmarkRunners/ConnectionPoolStressRunner.cs index 40f211c9b8..2595a7cdaf 100644 --- a/src/Microsoft.Data.SqlClient/tests/PerformanceTests/BenchmarkRunners/ConnectionPoolStressRunner.cs +++ b/src/Microsoft.Data.SqlClient/tests/PerformanceTests/BenchmarkRunners/ConnectionPoolStressRunner.cs @@ -322,11 +322,18 @@ public async Task MultiCommandReuse() /// wait. Measures how well the pool handles back-pressure when all connections are /// checked out and callers are queued. /// + /// + /// sets how far the pool is oversubscribed, and so how deep + /// the queue of waiting callers gets. It is the queue depth rather than the concurrency + /// level here, because saturating the pool already takes + /// tasks before any caller has to wait at all. + /// [Benchmark] public async Task PoolExhaustionRecovery() { - // Ensure we exceed pool capacity - int taskCount = Math.Max(Parallelism, MaxPoolSize * 2); + // Saturate the pool, then oversubscribe it by Parallelism so exactly that many + // callers are queued waiting for a connection to come back. + int taskCount = MaxPoolSize + Parallelism; var tasks = new Task[taskCount]; for (int i = 0; i < taskCount; i++) { From 2959100e6d3b43deeec56c4637717c8b9b2eef32 Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Mon, 24 Aug 2026 16:30:00 -0700 Subject: [PATCH 24/34] Rename RapidFireOpenClose to RapidFireOpenCloseAsync Pairs it with RapidFireOpenCloseSync so the two variants are named symmetrically. Updates the two cref references in ConnectionPoolChurnRunner and ConnectionPoolRampRunner that pointed at the old name. The old name exists on main, so a PR-vs-main comparison will not match this row. That is the correct outcome rather than a loss: the benchmark now pre-warms the pool where main clears it every iteration, so the two are no longer measuring the same thing and a delta between them would be spurious. compare_perf.py unions the baseline and current key sets and only computes a delta when both sides are present, so the unmatched row reports no delta instead of failing. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../BenchmarkRunners/ConnectionPoolChurnRunner.cs | 2 +- .../BenchmarkRunners/ConnectionPoolRampRunner.cs | 2 +- .../BenchmarkRunners/ConnectionPoolStressRunner.cs | 4 ++-- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/tests/PerformanceTests/BenchmarkRunners/ConnectionPoolChurnRunner.cs b/src/Microsoft.Data.SqlClient/tests/PerformanceTests/BenchmarkRunners/ConnectionPoolChurnRunner.cs index 27589752a8..3638760410 100644 --- a/src/Microsoft.Data.SqlClient/tests/PerformanceTests/BenchmarkRunners/ConnectionPoolChurnRunner.cs +++ b/src/Microsoft.Data.SqlClient/tests/PerformanceTests/BenchmarkRunners/ConnectionPoolChurnRunner.cs @@ -20,7 +20,7 @@ namespace Microsoft.Data.SqlClient.PerformanceTests /// concern for the new ChannelDbConnectionPool, which aims to avoid extra allocations /// on the hot path (issue #3356). /// - /// This overlaps in shape — + /// This overlaps in shape — /// the inner loop is identical — but not in purpose, and the two are not /// interchangeable. Being single-threaded, this runner has no scheduling or wake-up /// component, which makes it far more sensitive: its sync and async variants have diff --git a/src/Microsoft.Data.SqlClient/tests/PerformanceTests/BenchmarkRunners/ConnectionPoolRampRunner.cs b/src/Microsoft.Data.SqlClient/tests/PerformanceTests/BenchmarkRunners/ConnectionPoolRampRunner.cs index aa43a47b99..8af1d2cc28 100644 --- a/src/Microsoft.Data.SqlClient/tests/PerformanceTests/BenchmarkRunners/ConnectionPoolRampRunner.cs +++ b/src/Microsoft.Data.SqlClient/tests/PerformanceTests/BenchmarkRunners/ConnectionPoolRampRunner.cs @@ -19,7 +19,7 @@ namespace Microsoft.Data.SqlClient.PerformanceTests /// roughly N x connect latency. V2 has no such gate, so the opens overlap and total /// latency approaches a single connect. /// - /// Contrast with , which also + /// Contrast with , which also /// starts from a cold pool but releases each connection immediately. Because nothing is /// held, one physical connection can satisfy every caller in turn, so that benchmark /// rewards a pool that grows as slowly as possible and penalizes concurrent creation. diff --git a/src/Microsoft.Data.SqlClient/tests/PerformanceTests/BenchmarkRunners/ConnectionPoolStressRunner.cs b/src/Microsoft.Data.SqlClient/tests/PerformanceTests/BenchmarkRunners/ConnectionPoolStressRunner.cs index 2595a7cdaf..78ce2a74a3 100644 --- a/src/Microsoft.Data.SqlClient/tests/PerformanceTests/BenchmarkRunners/ConnectionPoolStressRunner.cs +++ b/src/Microsoft.Data.SqlClient/tests/PerformanceTests/BenchmarkRunners/ConnectionPoolStressRunner.cs @@ -155,7 +155,7 @@ public void Cleanup() /// /// [Benchmark] - public async Task RapidFireOpenClose() + public async Task RapidFireOpenCloseAsync() { // NOTE: the Math.Max floor dominates for most [Params] combinations, so total // checkouts do NOT scale cleanly with pool capacity — at Parallelism 20 and 25 both @@ -179,7 +179,7 @@ public async Task RapidFireOpenClose() } /// - /// Sync counterpart to : the same zero-hold churn, but + /// Sync counterpart to : the same zero-hold churn, but /// every checkout goes through the blocking Open() path. /// /// From 586bf918ec38495f5a5581e74e3e59927ae3004b Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Tue, 25 Aug 2026 09:49:09 -0700 Subject: [PATCH 25/34] Give ConnectionPoolChurnRunner a 60s connect timeout Setup now establishes up to a hundred physical connections back to back at the larger PoolDepth, and the default 15s connect timeout is tight for that against a loaded remote server. ConnectionPoolStressRunner already uses 60 for the same reason, since it pre-warms to MaxPoolSize. This cannot move the measurement. The pool is pre-warmed and driven by a single thread against MaxPoolSize=100, so no measured open ever waits for a connection, and the timeout only bounds waiting. Baseline and current get the same value. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../BenchmarkRunners/ConnectionPoolChurnRunner.cs | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/Microsoft.Data.SqlClient/tests/PerformanceTests/BenchmarkRunners/ConnectionPoolChurnRunner.cs b/src/Microsoft.Data.SqlClient/tests/PerformanceTests/BenchmarkRunners/ConnectionPoolChurnRunner.cs index 3638760410..3c13c9c59a 100644 --- a/src/Microsoft.Data.SqlClient/tests/PerformanceTests/BenchmarkRunners/ConnectionPoolChurnRunner.cs +++ b/src/Microsoft.Data.SqlClient/tests/PerformanceTests/BenchmarkRunners/ConnectionPoolChurnRunner.cs @@ -85,7 +85,11 @@ public void Setup() MaxPoolSize = 100, // Pin the floor at the requested depth so pruning cannot shrink the pool back // down mid-run and change what the benchmark is measuring. - MinPoolSize = PoolDepth + MinPoolSize = PoolDepth, + // Matches ConnectionPoolStressRunner. At the larger PoolDepth, setup establishes + // a hundred physical connections back to back, and the default 15s is tight for + // that against a loaded remote server. + ConnectTimeout = 60 }; _connectionString = builder.ConnectionString; From cd48acdaca1616da72772efe55d64eaa58881794 Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Tue, 25 Aug 2026 10:44:17 -0700 Subject: [PATCH 26/34] Make the v2 pool's sync wait visible to the thread pool The sync acquire path parked on a ManualResetEventSlim behind a static ProcessorCount/2 SemaphoreSlim. Both primitives block opaquely, so the thread pool never learned the thread was stalled. Because the idle channel is created without AllowSynchronousContinuations, the continuation that hands a waiter its connection is queued to the thread pool, so a saturated pool ended up waiting on work it had given the thread pool no reason to schedule. Block on the read's Task instead. Task-based blocking notifies the thread pool, which injects a worker immediately rather than after its slow starvation heuristic. Drop the semaphore: it never prevented sync work from consuming thread pool threads, since waiting on it blocks just as opaquely, and it starved the continuations its waiters depended on. Measured on a 10-core host with 50 sync waiters against a 3-connection pool, 1000 acquires: the old shape failed to complete 1000 ops in 20s despite the thread pool growing to 48 threads, because injected threads picked up queued work items ahead of the queued continuations. Blocking on the Task completes the same workload in 1.9s. Keeping the semaphore but fixing the wait still takes 19.3s, confirming the gate is a second instance of the same problem. Deadlock exposure is unchanged: with thread injection disabled, the old and new shapes both stall, so the semaphore was not providing the protection its comment claimed. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../connection-pooling.instructions.md | 15 ++++-- .../ConnectionPool/ChannelDbConnectionPool.cs | 49 +++++++------------ .../ConnectionPoolContentionRunner.cs | 2 +- 3 files changed, 31 insertions(+), 35 deletions(-) diff --git a/.github/instructions/connection-pooling.instructions.md b/.github/instructions/connection-pooling.instructions.md index 6464fdedd0..e845e2e079 100644 --- a/.github/instructions/connection-pooling.instructions.md +++ b/.github/instructions/connection-pooling.instructions.md @@ -124,13 +124,20 @@ private readonly ChannelWriter _idleConnectionWriter; await _idleConnectionReader.WaitToReadAsync(token); ``` -### Sync Over Async Protection +### Sync Over Async Blocking ```csharp -// Prevent thread pool starvation -private static SemaphoreSlim _syncOverAsyncSemaphore = - new(Math.Max(1, Environment.ProcessorCount / 2)); +// Block via Task, not an opaque primitive, so the thread pool learns this +// thread is stalled and can inject a worker to run the read's continuation. +return _idleChannel.ReadAsync(cancellationToken).AsTask().GetAwaiter().GetResult(); ``` +The idle channel is created without `AllowSynchronousContinuations`, so the continuation +that completes a pending read is queued to the thread pool. Blocking on an opaque primitive +(`ManualResetEventSlim`, `SemaphoreSlim`) hides the stall from the thread pool, so under +saturation every parked thread waits on a continuation the pool does not know it needs to +run. Do not reintroduce a semaphore around this wait: it cannot stop sync work from +consuming thread pool threads, since waiting on it blocks just as opaquely. + ## Best Practices ### Application Design diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs index 7a3404fa6a..2f24cbfe66 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs @@ -62,12 +62,6 @@ namespace Microsoft.Data.SqlClient.ConnectionPool internal sealed class ChannelDbConnectionPool : IDbConnectionPool, IDisposable { #region Fields - // Limits synchronous operations which depend on async operations on managed - // threads from blocking on all available threads, which would stop async tasks - // from being scheduled and cause deadlocks. Use ProcessorCount/2 as a balance - // between sync and async tasks. - private static SemaphoreSlim _syncOverAsyncSemaphore = new(Math.Max(1, Environment.ProcessorCount / 2)); - /// /// Tracks the number of instances of this class. Used to generate unique IDs for each instance. /// @@ -1792,30 +1786,25 @@ private void SweepEmancipatedConnections() /// The connection read from the channel. private DbConnectionInternal? ReadChannelSyncOverAsync(CancellationToken cancellationToken) { - // If there are no connections in the channel, then ReadAsync will block until one is available. - // Channels doesn't offer a sync API, so running ReadAsync synchronously on this thread may spawn - // additional new async work items in the managed thread pool if there are no items available in the - // channel. We need to ensure that we don't block all available managed threads with these child - // tasks or we could deadlock. Prefer to block the current user-owned thread, and limit throughput - // to the managed threadpool. - - _syncOverAsyncSemaphore.Wait(cancellationToken); - try - { - ConfiguredValueTaskAwaitable.ConfiguredValueTaskAwaiter awaiter = - _idleChannel.ReadAsync(cancellationToken).ConfigureAwait(false).GetAwaiter(); - using ManualResetEventSlim mres = new ManualResetEventSlim(false, 0); - - // Cancellation happens through the ReadAsync call, which will complete the task. - // Even a failed task will complete and set the ManualResetEventSlim. - awaiter.UnsafeOnCompleted(() => mres.Set()); - mres.Wait(CancellationToken.None); - return awaiter.GetResult(); - } - finally - { - _syncOverAsyncSemaphore.Release(); - } + // Channel exposes no synchronous blocking read, so a sync caller has to block on the + // async read. ValueTask cannot be blocked on directly (GetResult throws if the operation + // has not completed), so materialize a Task and block on that. + // + // Blocking through the Task is deliberate rather than incidental. The idle channel is + // created without AllowSynchronousContinuations, so the continuation that hands this + // thread its connection is queued to the thread pool instead of running inline on the + // returning thread. Task-based blocking notifies the thread pool that this thread is + // stalled, which lets it inject a worker to run that continuation promptly. Blocking on + // an opaque primitive (ManualResetEventSlim, SemaphoreSlim) hides the stall: when every + // pool thread is parked here, nothing runs the continuations until the thread pool's + // slow injection heuristic fires on its own, adding hundreds of milliseconds to each + // acquire while the pool is saturated. + // + // For the same reason there is no semaphore limiting how many threads may be in here at + // once. Such a gate cannot prevent sync work from consuming thread pool threads, because + // waiting on it blocks the calling thread just as opaquely; it only moves the invisible + // block one frame up and starves the very continuations these waiters depend on. + return _idleChannel.ReadAsync(cancellationToken).AsTask().GetAwaiter().GetResult(); } /// diff --git a/src/Microsoft.Data.SqlClient/tests/PerformanceTests/BenchmarkRunners/ConnectionPoolContentionRunner.cs b/src/Microsoft.Data.SqlClient/tests/PerformanceTests/BenchmarkRunners/ConnectionPoolContentionRunner.cs index e99875bb92..7da7be8529 100644 --- a/src/Microsoft.Data.SqlClient/tests/PerformanceTests/BenchmarkRunners/ConnectionPoolContentionRunner.cs +++ b/src/Microsoft.Data.SqlClient/tests/PerformanceTests/BenchmarkRunners/ConnectionPoolContentionRunner.cs @@ -113,7 +113,7 @@ public Task SteadyStateOpenQueryClose() conn.Open(); using var cmd = conn.CreateCommand(); cmd.CommandText = "SELECT 1"; - _ = cmd.ExecuteScalar(); + var result = cmd.ExecuteScalar(); // Dispose returns the connection to the pool. } }); From 968774a37cbf41e3588a59bfa61ba8e7916e8052 Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Tue, 25 Aug 2026 10:53:47 -0700 Subject: [PATCH 27/34] Document the ConfigureAwait invariant the sync pool wait depends on ReadChannelSyncOverAsync blocks on the idle channel read, which is the classic sync-over-async deadlock shape under a single-threaded SynchronizationContext. It is safe only because IdleConnectionChannel.ReadAsync awaits with ConfigureAwait(false), so the completion never needs the context the blocked thread is holding. Nothing enforced that, and the invariant lives in a different file from the code that depends on it. Record it at both ends. Comment-only. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../SqlClient/ConnectionPool/ChannelDbConnectionPool.cs | 5 +++++ .../Data/SqlClient/ConnectionPool/IdleConnectionChannel.cs | 7 +++++++ 2 files changed, 12 insertions(+) diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs index 2f24cbfe66..b2b69855b2 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs @@ -1804,6 +1804,11 @@ private void SweepEmancipatedConnections() // once. Such a gate cannot prevent sync work from consuming thread pool threads, because // waiting on it blocks the calling thread just as opaquely; it only moves the invisible // block one frame up and starves the very continuations these waiters depend on. + // + // Blocking on a Task is the classic sync-over-async deadlock shape when a single-threaded + // SynchronizationContext is installed (WPF, WinForms, legacy ASP.NET). It is safe here + // only because IdleConnectionChannel.ReadAsync awaits with ConfigureAwait(false), so its + // completion never needs the context this thread is holding. Do not remove that. return _idleChannel.ReadAsync(cancellationToken).AsTask().GetAwaiter().GetResult(); } diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/IdleConnectionChannel.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/IdleConnectionChannel.cs index 801c0e7c2f..df4c1cbc95 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/IdleConnectionChannel.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/IdleConnectionChannel.cs @@ -95,6 +95,13 @@ internal bool TryRead(out DbConnectionInternal? connection) /// Asynchronously reads a value from the channel. /// Decrements the idle count when a non-null connection is read. /// + /// + /// The ConfigureAwait(false) below is load bearing. Sync callers reach this method + /// through ChannelDbConnectionPool.ReadChannelSyncOverAsync, which blocks on the + /// returned operation. If this resumption captured a single-threaded + /// (WPF, WinForms, legacy ASP.NET), completing it would + /// require the very thread that is blocked waiting for it, deadlocking the caller. + /// internal async ValueTask ReadAsync(CancellationToken cancellationToken) { var connection = await _reader.ReadAsync(cancellationToken).ConfigureAwait(false); From 51525273bde557f98f77fd94d07af328d61e404f Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Tue, 25 Aug 2026 14:50:51 -0700 Subject: [PATCH 28/34] Tune ephemeral ports on the Windows perf harness MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The Linux harness widens ip_local_port_range and enables tcp_tw_reuse because the connection-churn benchmarks exhaust ephemeral ports (run-perf-tests.sh §2.9). run-perf-tests.ps1 declared that tuning "Linux-only and intentionally omitted", but it is Linux-only in mechanism, not in purpose. Untuned Windows allows 16,384 dynamic ports with a 120s TcpTimedWaitDelay, about 136 sustained connects/sec. The suite opens far more than that: SqlConnectionRunner has Pooling as a [Params(true, false)] axis, so its Pooling=False cases emit ~6,600 back-to-back physical connects, and the ramp and stress pool runners add ~5,800 more. The client drains the port range within seconds and then blocks waiting for sockets to leave TIME_WAIT. That lands directly in the reported numbers rather than being absorbed: runnerconfig.jsonc pins InvocationCount for every runner, so BenchmarkDotNet's pilot stage never runs and nothing renormalises wall-clock time. Windows results were measuring the harness's own port pressure alongside the driver, and were not comparable to Linux. Widen the dynamic port range via netsh to 10000-65535 (55,536 ports). The range starts at 10000 rather than the Linux harness's 1024 because on Windows the dynamic range also serves any bind(0), so reaching into the registered range risks colliding with services that bind fixed ports, 1433 among them. TcpTimedWaitDelay is reported rather than set: the stack reads it at boot so writing it could not help the current run, and unlike the Linux sysctl it would persist across reboots. The warning names the exact value to set at VM provisioning. The tuning is best-effort and never fails the run. netsh is invoked with the preference relaxed, since Windows PowerShell 5.1 promotes a non-elevated netsh's stderr to a terminating error under ErrorActionPreference='Stop'. Before/after port state and the effective TcpTimedWaitDelay are captured under results/diagnostics/ so a run records whether the tuning actually applied. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- eng/pipelines/perf/README.md | 29 ++++++- eng/pipelines/perf/scripts/run-perf-tests.ps1 | 85 ++++++++++++++++++- 2 files changed, 110 insertions(+), 4 deletions(-) diff --git a/eng/pipelines/perf/README.md b/eng/pipelines/perf/README.md index faaeed0a23..09ddfe1831 100644 --- a/eng/pipelines/perf/README.md +++ b/eng/pipelines/perf/README.md @@ -325,12 +325,39 @@ supplies the isolated dedicated host, the tuned SQL instance, and the disjoint c | Fail loud | Preflight `SELECT 1` before any pass, **and** a post-pass guard that fails the run if a pass produced **zero** benchmark results — so an empty comparison can never be reported green. | | Warm-up | Touches the target DB in the preflight to warm the buffer pool / plan cache before the first measured benchmark. | | Allocator tuning (Linux) | Exports `MALLOC_MMAP_THRESHOLD_=128MiB` and `MALLOC_TRIM_THRESHOLD_=-1` so large-buffer benches (`LargeDataRead`, `SqlBulkCopy`) stop re-`mmap`ing per iteration. | -| Network tuning (Linux) | Best-effort `sysctl` to widen the ephemeral port range and enable `tcp_tw_reuse` for churn benches (`ConnectionPoolStress`, `ParallelAsyncConnection`). Never fails the run. | +| Network tuning | Best-effort widening of the ephemeral port range so connection-churn benches don't stall on `TIME_WAIT`. On Linux: `sysctl` for `ip_local_port_range` plus `tcp_tw_reuse`. On Windows: `netsh int ipv4 set dynamicport tcp` (10000-65535) plus `TcpTimedWaitDelay` (reboot-scoped, so it benefits subsequent runs). Never fails the run. See [Ephemeral ports](#ephemeral-ports-and-connection-churn). | | Diagnostics | Writes `results/diagnostics/`: SQL instance config (MAXDOP, memory, affinity, tempdb files, `@@VERSION`), host CPU topology, and per-pass CPU-clock/thermal telemetry (before/after each pass). | | Regression gate | `failOnRegression` threads `--fail-on-regression`; only a **candidate-slower** delta past the threshold fails, and in interleaved mode only after best-of-N confirmation. Default off. | | Interleaving | In `interleaved` mode the harness runs **one benchmark unit at a time, baseline then candidate back-to-back**, so both sides see the same host state (see below). | | Best-of-N confirmation | A unit flagged in the first interleaved pass is re-run `confirmationRuns` times; a regression is **confirmed** only on a strict majority. Unconfirmed flags are reported but never fail the gate. | +### Ephemeral ports and connection churn + +The suite opens far more *physical* TCP connections than the benchmark names suggest, so the client's +ephemeral port supply is a real constraint rather than a theoretical one: + +| Source | Physical connects | Why | +| ------ | ----------------- | --- | +| `SqlConnectionRunner` (`Pooling=False` cases) | ~6,600 | `Pooling` is a `[Params(true, false)]` axis, so half the cases bypass the pool entirely and every operation is a fresh connect. 4 cases x (5 warmup + 50 iterations) x 30 invocations, back-to-back with no delay. | +| `ConnectionPoolRampRunner` | ~2,700 | `[IterationSetup]` calls `ClearAllPools()`, so every iteration ramps from a cold pool. | +| `ConnectionPoolStressRunner` | ~3,100 | `MinPoolSize = MaxPoolSize`, so each of the 42 cases fills a fresh pool. | + +Every closed socket then holds its port in `TIME_WAIT`. Untuned Windows allows 16,384 dynamic ports +(49152-65535) with a 120 s `TcpTimedWaitDelay` — about **136 sustained connects/sec**, which the +suite exceeds by orders of magnitude. Once the range drains, each subsequent connect blocks until a +port is released. + +This matters more here than in a typical harness because `runnerconfig.jsonc` pins `InvocationCount` +for every runner. That disables BenchmarkDotNet's pilot stage, so nothing renormalises wall-clock +time: a port stall lands directly in the reported per-operation numbers rather than being absorbed by +a smaller invocation count. + +Both platforms are tuned for this, and results are **not comparable across platforms without it** — +an untuned Windows run measures its own port pressure as much as it measures the driver. If a Windows +run looks uniformly slow, check `results/diagnostics/tcp-dynamicport.txt` to confirm the widening +applied (it needs an elevated agent), and sample `netstat -an | find /c "TIME_WAIT"` during the run: +a count parked near the top of the range means the client is port-starved. + ### Interleaving + best-of-N (run model) `benchmarkRunMode` selects how the two variants are measured: diff --git a/eng/pipelines/perf/scripts/run-perf-tests.ps1 b/eng/pipelines/perf/scripts/run-perf-tests.ps1 index 57b2091090..aad97b5b60 100644 --- a/eng/pipelines/perf/scripts/run-perf-tests.ps1 +++ b/eng/pipelines/perf/scripts/run-perf-tests.ps1 @@ -270,14 +270,93 @@ if ($sqlcmd) { # # The Perf Test Lab already provides the isolated dedicated host, the tuned SQL instance and the # disjoint client CPU set (PERF_CLIENT_CPUS, pinned per pass below). These are the remaining -# harness-owned controls: per-run diagnostics, a fail-loud preflight and a warm-up, so a run's -# mean/variance is steadier and a broken run cannot masquerade as a pass. (The glibc allocator and -# sysctl tuning from the Linux harness are Linux-only and intentionally omitted here.) +# harness-owned controls: network tuning, per-run diagnostics, a fail-loud preflight and a warm-up, +# so a run's mean/variance is steadier and a broken run cannot masquerade as a pass. (The glibc +# allocator tuning from the Linux harness is genuinely glibc-specific and is omitted here; the +# ephemeral-port tuning is NOT Linux-specific and has a Windows equivalent -- see §2.9 below.) #################################################################################################### $DiagDir = Join-Path $ResultsDir "diagnostics" New-Item -ItemType Directory -Force -Path $DiagDir | Out-Null +# --- §2.9 Network tuning (best-effort; needs elevation, so it must never fail the run) ------------ +# Connection-churn benches exhaust ephemeral ports, and the churn is far heavier than it looks: +# SqlConnectionRunner has [Params(true, false)] on Pooling, so its Pooling=False cases open a fresh +# physical TCP connection for every single operation -- 4 cases x (5 warmup + 50 iterations) x 30 +# invocations = ~6,600 back-to-back connects with no delay between them. ConnectionPoolRampRunner +# adds ~2,700 more (its [IterationSetup] calls ClearAllPools(), so every iteration ramps from cold) +# and ConnectionPoolStressRunner ~3,100 (MinPoolSize = MaxPoolSize, so all 42 cases fill a fresh +# pool). Every one of those closed sockets then sits in TIME_WAIT holding its port. +# +# Windows defaults to 16,384 dynamic ports (49152-65535) with a 120s TcpTimedWaitDelay, i.e. only +# ~136 sustained connects/sec. The suite wants orders of magnitude more, so without this the client +# drains the port pool within seconds and every subsequent connect blocks waiting for a socket to +# leave TIME_WAIT. That inflates socket setup latency enormously -- and because runnerconfig.jsonc +# pins InvocationCount for every runner, BenchmarkDotNet's pilot stage never runs and the stall is +# not normalised away; it lands directly in the reported per-operation numbers. The Linux harness +# fixes this with ip_local_port_range + tcp_tw_reuse (run-perf-tests.sh §2.9); this is the Windows +# equivalent, so the two platforms measure the driver rather than their own port pressure. +# +# The range starts at 10000 rather than the Linux harness's 1024: on Windows the dynamic range is +# also the range the stack allocates from for any bind(0), and reaching down into the registered +# range risks colliding with a service that expects to bind a fixed port (SQL Server's own 1433 +# among them, when the client and server share a host). 10000-65535 still yields 55,536 ports, +# ~3.4x the default, which is comfortably past what the suite consumes. +# +# netsh is invoked with the preference relaxed for the duration of the call: on Windows PowerShell +# 5.1 a non-elevated netsh writes "requires elevation" to stderr, which $ErrorActionPreference='Stop' +# would otherwise promote to a terminating error (see "Native-command error handling" above). This +# tuning is best-effort by design, so it reports and continues rather than failing the run. +$previousPreference = $ErrorActionPreference +$ErrorActionPreference = 'Continue' +try { + $portsBefore = (netsh int ipv4 show dynamicport tcp 2>&1 | Out-String).Trim() + $LASTEXITCODE = 0 + netsh int ipv4 set dynamicport tcp start=10000 num=55536 2>&1 | Out-Null + if ($LASTEXITCODE -eq 0) { + Write-Host "Widened the TCP dynamic port range to 10000-65535 (55,536 ephemeral ports)." + } else { + Write-Warning ("Could not widen the TCP dynamic port range (netsh exit $LASTEXITCODE); " + + "connection-churn benchmarks may stall on TIME_WAIT and the run's " + + "socket-setup latency will be inflated. Is the agent elevated?") + } + $portsAfter = (netsh int ipv4 show dynamicport tcp 2>&1 | Out-String).Trim() + "=== before ===`n$portsBefore`n`n=== after ===`n$portsAfter" | + Out-File -FilePath (Join-Path $DiagDir "tcp-dynamicport.txt") -Encoding UTF8 +} catch { + Write-Warning "Could not tune the TCP dynamic port range: $_" +} finally { + $ErrorActionPreference = $previousPreference +} + +# TcpTimedWaitDelay (seconds a closed socket lingers in TIME_WAIT; valid range 30-300, default 120) +# has no immediate-effect equivalent to Linux's tcp_tw_reuse: the stack reads it at boot, so writing +# it here could not help the current run. It is also a persistent, machine-wide change, which is a +# wider blast radius than the Linux harness's 'sysctl -w' (that resets at reboot). So report it +# rather than set it, and leave the decision to VM provisioning. +try { + $tcpParams = 'HKLM:\SYSTEM\CurrentControlSet\Services\Tcpip\Parameters' + $tcpProps = Get-ItemProperty -Path $tcpParams -ErrorAction SilentlyContinue + # StrictMode makes a missing-property access a terminating error, so probe before reading. + $effective = if ($null -ne $tcpProps -and + ($tcpProps.PSObject.Properties.Name -contains 'TcpTimedWaitDelay')) { + $tcpProps.TcpTimedWaitDelay + } else { + $null + } + $effectiveText = if ($null -ne $effective) { "$effective s" } else { "unset (stack default, 120 s)" } + "TcpTimedWaitDelay in effect for this run: $effectiveText" | + Out-File -FilePath (Join-Path $DiagDir "tcp-timedwaitdelay.txt") -Encoding UTF8 + if ($null -eq $effective -or $effective -gt 60) { + Write-Warning ("TcpTimedWaitDelay is $effectiveText. Closed sockets hold their port for " + + "that long, which caps sustained connects even with the widened range. " + + "To lower it, set HKLM:\SYSTEM\CurrentControlSet\Services\Tcpip\Parameters" + + "\TcpTimedWaitDelay (DWORD) to 30 at VM provisioning; it takes effect on reboot.") + } +} catch { + Write-Warning "Could not read TcpTimedWaitDelay: $_" +} + # --- §2.11 Capture host CPU topology (static, once per run) --------------------------------------- try { Get-CimInstance Win32_Processor | From e41a3b3c5bc8b20198065a029f39ddd72c6c87e9 Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Tue, 25 Aug 2026 16:55:25 -0700 Subject: [PATCH 29/34] Expand connection pool benchmark parameter coverage The pool runners' parameters were concentrated in one demand/capacity regime. Classifying every (Parallelism, MaxPoolSize) cell by ratio gave 5 deeply under-subscribed, 3 under-subscribed, 2 balanced and 1 over-subscribed -- and both confirmed pool regressions live in that single over-subscribed cell. Rebalance toward the regime that actually discriminates, paying for it by trimming a redundant axis rather than by spending more wall time. Churn: add PoolDepth 10. Depths 1 and 100 flip the sign of the result but cannot distinguish a threshold from a gradient in reuse locality, and those imply different fixes. Contention: add MaxPoolSize 25 and 200, giving ratios 0.25/0.5/1/2/5. The intermediate over-subscribed step shows how fast cost grows once the pool starts running dry; 200 is the most commonly configured explicit Max Pool Size in production. Stress: replace Parallelism 10/20/25 with 10/50. The old ladder spanned 2.5x and 20 vs 25 largely re-measured the same regime, while this runner carries seven benchmarks so the axis is expensive. Frees 14 cases, widens the span to 5x, and adds a fully-subscribed cell. Ramp: add Parallelism 100 to extend the scaling curve, and raise MaxPoolSize to 200 to preserve the runner's documented invariant that capacity always exceeds parallelism. ThreadPoolPressure: source MinWorkerThreads from ProcessorCount multiples instead of the constants 8 and 128. The value is only meaningful relative to ProcessorCount, since that is both the default floor it displaces and the runtime's immediate cooperative-blocking injection budget. On a 16-core host 8 is below the default -- a configuration Microsoft cautions against -- and on a 4-core host it would stop starving anything at all while still reporting numbers. Also correct two stale claims in that runner's remarks. Injection has not been limited to "one or two threads per second" since .NET 6, which compensates for cooperative blocking with up to one thread per processor immediately and then 25ms steps capped at 250ms; the suite targets net8.0 and later. And starvation is not only a misconfiguration: the default minimum is ProcessorCount, that honours cgroup quotas, and ASP.NET Core never raises it, so a 1-2 vCPU container runs a floor of 1 or 2 by default. Net effect is 69 -> 67 cases, so coverage improves without growing the run. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../ConnectionPoolChurnRunner.cs | 7 +- .../ConnectionPoolContentionRunner.cs | 23 +++++-- .../ConnectionPoolRampRunner.cs | 4 +- .../ConnectionPoolStressRunner.cs | 21 ++++-- .../ConnectionPoolThreadPoolPressureRunner.cs | 68 ++++++++++++++++--- 5 files changed, 96 insertions(+), 27 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/tests/PerformanceTests/BenchmarkRunners/ConnectionPoolChurnRunner.cs b/src/Microsoft.Data.SqlClient/tests/PerformanceTests/BenchmarkRunners/ConnectionPoolChurnRunner.cs index 3c13c9c59a..d2b94cea14 100644 --- a/src/Microsoft.Data.SqlClient/tests/PerformanceTests/BenchmarkRunners/ConnectionPoolChurnRunner.cs +++ b/src/Microsoft.Data.SqlClient/tests/PerformanceTests/BenchmarkRunners/ConnectionPoolChurnRunner.cs @@ -64,8 +64,13 @@ public class ConnectionPoolChurnRunner : BaseRunner /// the V2 pool cycles through every idle connection in turn, touching all of their /// buffers and parser state. Depth is therefore the axis that exposes reuse locality, /// and measuring only depth 1 would hide it entirely. + /// + /// The middle depth is what makes the axis diagnostic rather than merely directional. + /// Depths 1 and 100 alone show only that reuse locality degrades somewhere in between; + /// they cannot distinguish a threshold (cost appears once the pool exceeds some working + /// set) from a gradient (cost grows smoothly with depth). Those imply different fixes. /// - [Params(1, 100)] + [Params(1, 10, 100)] public int PoolDepth { get; set; } private string _connectionString; diff --git a/src/Microsoft.Data.SqlClient/tests/PerformanceTests/BenchmarkRunners/ConnectionPoolContentionRunner.cs b/src/Microsoft.Data.SqlClient/tests/PerformanceTests/BenchmarkRunners/ConnectionPoolContentionRunner.cs index 7da7be8529..c13bdb4d3d 100644 --- a/src/Microsoft.Data.SqlClient/tests/PerformanceTests/BenchmarkRunners/ConnectionPoolContentionRunner.cs +++ b/src/Microsoft.Data.SqlClient/tests/PerformanceTests/BenchmarkRunners/ConnectionPoolContentionRunner.cs @@ -49,13 +49,24 @@ public class ConnectionPoolContentionRunner : BaseRunner public int Parallelism { get; set; } /// - /// Maximum pool size, exercised across three regimes relative to - /// : larger than the worker count (idle spare - /// connections, no contention), equal to it (fully subscribed, no contention), and - /// smaller than it (pool exhaustion forces workers to wait for a connection to be - /// returned — back-pressure). + /// Maximum pool size, exercised across the full demand/capacity ladder relative to + /// : larger than the worker count (idle spare connections, no + /// contention), equal to it (fully subscribed, no contention), and progressively smaller + /// than it (pool exhaustion forces workers to wait for a connection to be returned — + /// back-pressure). /// - [Params(100, 50, 10)] + /// + /// At 50 these give demand/capacity ratios of 0.25, 0.5, 1, 2 + /// and 5. The intermediate over-subscribed step (ratio 2) is deliberate: back-pressure + /// regressions show up only once demand exceeds capacity, and without a point between + /// "fully subscribed" and "five times over" there is no way to tell how quickly the cost + /// grows once the pool starts running dry. + /// + /// 200 is included because it is the most commonly configured explicit Max Pool Size in + /// production, so the least-contended row corresponds to a real deployment rather than + /// only to the driver default. + /// + [Params(200, 100, 50, 25, 10)] public int MaxPoolSize { get; set; } /// diff --git a/src/Microsoft.Data.SqlClient/tests/PerformanceTests/BenchmarkRunners/ConnectionPoolRampRunner.cs b/src/Microsoft.Data.SqlClient/tests/PerformanceTests/BenchmarkRunners/ConnectionPoolRampRunner.cs index 8af1d2cc28..344884fbc3 100644 --- a/src/Microsoft.Data.SqlClient/tests/PerformanceTests/BenchmarkRunners/ConnectionPoolRampRunner.cs +++ b/src/Microsoft.Data.SqlClient/tests/PerformanceTests/BenchmarkRunners/ConnectionPoolRampRunner.cs @@ -44,14 +44,14 @@ public class ConnectionPoolRampRunner : BaseRunner /// its connection until all of them have connected, so the pool must open exactly /// this many physical connections. /// - [Params(10, 25, 50)] + [Params(10, 25, 50, 100)] public int Parallelism { get; set; } /// /// Max pool size. Deliberately larger than every value so /// the ramp is never bounded by pool capacity. /// - [Params(100)] + [Params(200)] public int MaxPoolSize { get; set; } private string _connectionString; diff --git a/src/Microsoft.Data.SqlClient/tests/PerformanceTests/BenchmarkRunners/ConnectionPoolStressRunner.cs b/src/Microsoft.Data.SqlClient/tests/PerformanceTests/BenchmarkRunners/ConnectionPoolStressRunner.cs index 78ce2a74a3..4c4c7e63d9 100644 --- a/src/Microsoft.Data.SqlClient/tests/PerformanceTests/BenchmarkRunners/ConnectionPoolStressRunner.cs +++ b/src/Microsoft.Data.SqlClient/tests/PerformanceTests/BenchmarkRunners/ConnectionPoolStressRunner.cs @@ -34,7 +34,13 @@ public class ConnectionPoolStressRunner : BaseRunner /// /// Number of concurrent tasks hammering the pool. /// - [Params(10, 20, 25)] + /// + /// Two values spanning 5x rather than a tighter cluster. Every benchmark here runs at + /// every , so this axis is multiplied across seven benchmarks + /// and intermediate values buy far less than they cost — adjacent values in a narrow + /// range mostly re-measure the same regime. + /// + [Params(10, 50)] public int Parallelism { get; set; } /// @@ -43,13 +49,14 @@ public class ConnectionPoolStressRunner : BaseRunner /// /// The pool is pre-warmed to this many connections and pinned there by an equal /// Min Pool Size, so this is the number of connections actually resident for the - /// whole run rather than just a ceiling. Note that every value - /// is below every value here, so the limit itself is only reached by - /// , which deliberately oversubscribes it. For the - /// other benchmarks the pool never saturates, so this parameter mostly varies how many - /// idle connections the checkout path is choosing among. Treat a spread between two + /// whole run rather than just a ceiling. The limit itself is only reached by + /// , which deliberately oversubscribes it, and by the + /// fully-subscribed corner where equals this value. Everywhere + /// else the pool never saturates, so this parameter mostly varies how many idle + /// connections the checkout path is choosing among; treat a spread between two /// MaxPoolSize values at the same Parallelism as a noise estimate rather than a real - /// effect. + /// effect. The fully-subscribed corner is the exception — there the spread is a real + /// effect, because one side has idle spares to choose from and the other has none. /// [Params(50, 100)] public int MaxPoolSize { get; set; } diff --git a/src/Microsoft.Data.SqlClient/tests/PerformanceTests/BenchmarkRunners/ConnectionPoolThreadPoolPressureRunner.cs b/src/Microsoft.Data.SqlClient/tests/PerformanceTests/BenchmarkRunners/ConnectionPoolThreadPoolPressureRunner.cs index b3a158d2f7..187fb72d86 100644 --- a/src/Microsoft.Data.SqlClient/tests/PerformanceTests/BenchmarkRunners/ConnectionPoolThreadPoolPressureRunner.cs +++ b/src/Microsoft.Data.SqlClient/tests/PerformanceTests/BenchmarkRunners/ConnectionPoolThreadPoolPressureRunner.cs @@ -3,6 +3,8 @@ // See the LICENSE file in the project root for more information. using System; +using System.Collections.Generic; +using System.Linq; using System.Threading; using System.Threading.Tasks; using BenchmarkDotNet.Attributes; @@ -16,8 +18,14 @@ namespace Microsoft.Data.SqlClient.PerformanceTests /// A sync Open() against a saturated pool blocks its thread. When those threads /// are threadpool threads, a pool whose waiter wake-up requires a queued continuation /// cannot make progress: every thread is blocked in a wait, so the wake-up sits in the - /// queue until the threadpool injects another thread. Injection is rate limited to - /// roughly one or two threads per second, so each stall costs about a second. + /// queue until the threadpool injects another thread. + /// + /// Injection is not a single mechanism. Since .NET 6 the runtime recognises cooperative + /// blocking and compensates on a fast path — up to one thread per processor immediately + /// with no delay, then in 25ms steps capped at 250ms. Only once that budget is spent does + /// the caller fall back to the gate thread's starvation detection, which adds one thread + /// per 500ms cycle. Stalls here are therefore tens to a few hundred milliseconds, not the + /// whole seconds the pre-.NET 6 gate-thread-only path would have cost. /// /// covers the same shape at the default /// threadpool floor, which makes it dependent on hill-climbing timing and therefore @@ -33,11 +41,13 @@ namespace Microsoft.Data.SqlClient.PerformanceTests /// The effect is tail latency, not a shifted median, so compare distributions rather /// than means alone. /// - /// A regression here is a statement about application configuration, not a pool defect. - /// An application that blocks more thread pool threads than the thread pool has workers - /// is already misconfigured, and pre-warming the thread pool is the application's - /// responsibility rather than the driver's. This runner exists to characterise where - /// that boundary is and to catch it moving, not to drive the delta to zero. + /// A regression here is largely a statement about application configuration rather than a + /// pool defect, but the starved configuration is not exotic. The threadpool's default + /// minimum is , and that honours cgroup CPU quotas, + /// so a service in a 1-2 vCPU container runs with a floor of 1 or 2 by default and ASP.NET + /// Core never raises it. Blocking more threads than the floor is the common case for sync + /// callers, not a misconfiguration they opted into. This runner exists to characterise + /// where that boundary is and to catch it moving, not to drive the delta to zero. /// /// The pool implementation (legacy vs V2) is a process-level choice - see the remarks on /// . Run twice (UseConnectionPoolV2 false then @@ -62,13 +72,49 @@ public class ConnectionPoolThreadPoolPressureRunner : BaseRunner public int MaxPoolSize { get; set; } /// - /// Threadpool minimum worker thread count, pinned for the duration of the run. The - /// low value is below (starved); the high value is above - /// it (control). + /// Threadpool minimum worker thread count, pinned for the duration of the run. /// - [Params(8, 128)] + /// + /// Sourced from rather than fixed constants, + /// because this number is only meaningful relative to + /// — see that property for why. + /// + [ParamsSource(nameof(MinWorkerThreadsValues))] public int MinWorkerThreads { get; set; } + /// + /// The minimum worker counts to sweep, expressed as multiples of + /// . + /// + /// + /// Fixed constants would not survive a change of machine. The processor count is both + /// the value this parameter displaces (it is the runtime's own default floor) and the + /// size of the runtime's immediate cooperative-blocking injection budget, so the same + /// absolute number means "starved" on one host and "generous" on another. A constant + /// chosen to starve a 16-core benchmark machine would quietly stop starving anything on + /// a 4-core developer box, and the benchmark would keep reporting numbers that no longer + /// measure the wake path. + /// + /// The multiples map onto real deployments: a quarter of the processor count stands in + /// for the default floor of a 2-4 vCPU container, 1x is the runtime default that almost + /// every application actually runs, 2x is the most common explicit multiplier in shipped + /// code, and 8x is the control — comfortably above so + /// injection never gates progress. + /// + /// Floored at 1 because SetMinThreads rejects 0, and de-duplicated because the + /// lower multiples collapse together on very small hosts. + /// + public static IEnumerable MinWorkerThreadsValues => + new[] + { + Environment.ProcessorCount / 4, + Environment.ProcessorCount, + Environment.ProcessorCount * 2, + Environment.ProcessorCount * 8, + } + .Select(static value => Math.Max(1, value)) + .Distinct(); + /// /// Number of open/query/close operations each worker performs per invocation. /// From 8e5e8316fa802beb767ce11672bcb9a6d1b3b0d5 Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Wed, 26 Aug 2026 11:34:05 -0700 Subject: [PATCH 30/34] Make the perf harness self-diagnosing on long runs The Windows perf job overruns its 6h budget while the equivalent Linux job completes, and the run produces nothing that says why: the harness reports per-operation benchmark numbers but never records how long each unit took, and it waits on each unit with an unbounded wait(), so a single wedged unit silently consumes the whole budget and the job is killed with no indication of which unit was responsible. - Record wall-clock per (unit, variant, rep) and emit a slowest-first summary plus diagnostics/unit-timings.json. This separates a uniform environmental slowdown from time concentrated in particular units, which the benchmark results alone cannot show. - Add a per-unit timeout backstop (--unit-timeout-mins / PERF_UNIT_TIMEOUT_MINS, default 45). On expiry the process tree is killed, the log is tailed, and the run fails naming the unit. The tree is killed rather than just the child so orphans do not hold SQL connections open into the next unit. - Emit the timing report from a finally block, so it is produced on the failure path too, which is where it matters most. Also fix Windows CPU pinning, which could not have been working as reported. SetProcessAffinityMask was called without argtypes, so ctypes marshalled the 64-bit HANDLE as a C int and truncated it. Declare the signatures, read the effective mask back via GetProcessAffinityMask, and report what was actually applied. On failure, report the system mask so a CPU set chosen for a larger machine is identified as such instead of being swallowed. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- eng/pipelines/perf/scripts/interleave_perf.py | 223 ++++++++++++++++-- 1 file changed, 202 insertions(+), 21 deletions(-) diff --git a/eng/pipelines/perf/scripts/interleave_perf.py b/eng/pipelines/perf/scripts/interleave_perf.py index fdf66a5913..5c5739168d 100644 --- a/eng/pipelines/perf/scripts/interleave_perf.py +++ b/eng/pipelines/perf/scripts/interleave_perf.py @@ -33,6 +33,7 @@ import shutil import subprocess import sys +import time SCRIPT_DIR = os.path.dirname(os.path.abspath(__file__)) @@ -69,15 +70,26 @@ def parse_cpus(spec): def apply_affinity(proc, cpus): - """Pin *proc* to *cpus*. Never raises — pinning is an optimisation, not a gate.""" + """Pin *proc* to *cpus*. Never raises — pinning is an optimisation, not a gate. + + Returns a short human-readable description of what actually happened, so callers can + record it: a client that silently failed to pin (or pinned to fewer cores than intended) + changes throughput enough to look like a driver regression, and that must be visible in + the run log rather than inferred afterwards. + """ if not cpus: - return + return "not pinned (no CPU set given)" try: if hasattr(os, "sched_setaffinity"): # Linux os.sched_setaffinity(proc.pid, set(cpus)) - return + effective = sorted(os.sched_getaffinity(proc.pid)) + if set(effective) != set(cpus): + print(f"WARNING: pid {proc.pid} requested CPUs {cpus} but is running on " + f"{effective}.", file=sys.stderr) + return f"pinned to {len(effective)} core(s): {_fmt_cpus(effective)}" if os.name == "nt": # Windows import ctypes + from ctypes import wintypes # SetProcessAffinityMask takes a single-word mask that only addresses CPUs 0-63; # higher indices require processor-group APIs. Rather than set a mask that would @@ -86,29 +98,87 @@ def apply_affinity(proc, cpus): print(f"WARNING: CPU index >= 64 in {cpus}; SetProcessAffinityMask cannot address " f"processor groups, so pid {proc.pid} runs without CPU pinning.", file=sys.stderr) - return + return "not pinned (CPU index >= 64; needs processor-group APIs)" + + kernel32 = ctypes.WinDLL("kernel32", use_last_error=True) + # Declare the signatures explicitly. Without argtypes ctypes marshals a Python int + # as a C int, which truncates both the 64-bit HANDLE and a mask that addresses CPUs + # >= 32 -- the call then either fails outright or pins to the wrong cores. + kernel32.SetProcessAffinityMask.argtypes = [wintypes.HANDLE, ctypes.c_size_t] + kernel32.SetProcessAffinityMask.restype = wintypes.BOOL + kernel32.GetProcessAffinityMask.argtypes = [ + wintypes.HANDLE, ctypes.POINTER(ctypes.c_size_t), ctypes.POINTER(ctypes.c_size_t)] + kernel32.GetProcessAffinityMask.restype = wintypes.BOOL + mask = 0 for c in cpus: mask |= (1 << c) - handle = int(proc._handle) # noqa: SLF001 (Popen exposes the OS handle here) - if ctypes.windll.kernel32.SetProcessAffinityMask(handle, ctypes.c_size_t(mask)) == 0: - print(f"WARNING: SetProcessAffinityMask failed for pid {proc.pid}.", file=sys.stderr) + + handle = wintypes.HANDLE(int(proc._handle)) # noqa: SLF001 (Popen exposes it here) + if not kernel32.SetProcessAffinityMask(handle, ctypes.c_size_t(mask)): + err = ctypes.get_last_error() + # A system affinity mask that does not contain every requested CPU is the + # common cause here (ERROR_INVALID_PARAMETER, 87): the CPU set was chosen for + # a machine with more cores than this one actually has. + sys_mask = ctypes.c_size_t(0) + proc_mask = ctypes.c_size_t(0) + detail = "" + if kernel32.GetProcessAffinityMask(handle, ctypes.byref(proc_mask), + ctypes.byref(sys_mask)): + ncpu = bin(sys_mask.value).count("1") + detail = (f" This machine has {ncpu} usable core(s) " + f"(system mask 0x{sys_mask.value:X}); requested mask 0x{mask:X}.") + print(f"WARNING: SetProcessAffinityMask failed for pid {proc.pid} " + f"(error {err}).{detail}", file=sys.stderr) + return f"NOT pinned (SetProcessAffinityMask failed, error {err}){detail}" + + proc_mask = ctypes.c_size_t(0) + sys_mask = ctypes.c_size_t(0) + if kernel32.GetProcessAffinityMask(handle, ctypes.byref(proc_mask), + ctypes.byref(sys_mask)): + effective = [i for i in range(64) if proc_mask.value & (1 << i)] + return f"pinned to {len(effective)} core(s): {_fmt_cpus(effective)}" + return f"pinned to {len(cpus)} core(s): {_fmt_cpus(cpus)}" except Exception as exc: # noqa: BLE001 print(f"WARNING: could not pin pid {getattr(proc, 'pid', '?')} to CPUs {cpus}: {exc}", file=sys.stderr) + return f"NOT pinned ({exc})" + return "not pinned (unsupported platform)" + + +def _fmt_cpus(cpus): + """Render a sorted CPU list compactly as ranges, e.g. [0,1,2,5] -> '0-2,5'.""" + if not cpus: + return "" + cpus = sorted(cpus) + parts = [] + start = prev = cpus[0] + for c in cpus[1:] + [None]: + if c is not None and c == prev + 1: + prev = c + continue + parts.append(str(start) if start == prev else f"{start}-{prev}") + if c is not None: + start = prev = c + return ",".join(parts) # -------------------------------------------------------------------------------------------------- # Running one unit and collecting its artifacts. # -------------------------------------------------------------------------------------------------- -def run_unit_process(exe_dir, assembly, unit, cwd, cpus, log_path, env_overrides=None): +def run_unit_process(exe_dir, assembly, unit, cwd, cpus, log_path, env_overrides=None, + timeout_secs=0): """Run one benchmark *unit* from the build at *exe_dir* in *cwd*. - Returns the subprocess return code. Kept as a small seam so tests can substitute - a fake runner. *env_overrides*, when given, is applied on top of the inherited - environment (e.g. a per-variant RUNNER_CONFIG so baseline and current can run with - different SqlClient behaviour flags, such as comparing the legacy vs new connection - pool from the SAME build). + Returns ``(returncode, elapsed_secs, affinity_note, timed_out)``. Kept as a small seam so + tests can substitute a fake runner. *env_overrides*, when given, is applied on top of the + inherited environment (e.g. a per-variant RUNNER_CONFIG so baseline and current can run with + different SqlClient behaviour flags, such as comparing the legacy vs new connection pool + from the SAME build). + + *timeout_secs* (0 disables) is a per-unit backstop. Without it a single wedged unit + consumes the entire pipeline budget and the job is killed with no indication of which unit + was responsible; with it the unit is killed, its log is tailed, and the run fails naming it. """ os.makedirs(cwd, exist_ok=True) cmd = ["dotnet", os.path.join(exe_dir, assembly)] @@ -118,11 +188,42 @@ def run_unit_process(exe_dir, assembly, unit, cwd, cpus, log_path, env_overrides if env_overrides: env.update(env_overrides) + started = time.monotonic() + timed_out = False with open(log_path, "w", encoding="utf-8") as log: proc = subprocess.Popen(cmd, cwd=cwd, env=env, stdout=log, stderr=subprocess.STDOUT) - apply_affinity(proc, cpus) - return proc.wait() + affinity_note = apply_affinity(proc, cpus) + 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 + + +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() + except Exception as exc: # noqa: BLE001 + print(f"WARNING: could not kill pid {getattr(proc, 'pid', '?')}: {exc}", file=sys.stderr) def _report_files(root): @@ -160,7 +261,8 @@ class Runner: """Holds the invariant run parameters and performs interleaved unit passes.""" def __init__(self, baseline_dir, current_dir, assembly, work_dir, cpus, - baseline_runner_config=None, current_runner_config=None): + baseline_runner_config=None, current_runner_config=None, + unit_timeout_secs=0): self.baseline_dir = baseline_dir self.current_dir = current_dir self.assembly = assembly @@ -171,6 +273,12 @@ def __init__(self, baseline_dir, current_dir, assembly, work_dir, cpus, # same build). None means "no override" -> both variants use the ambient RUNNER_CONFIG. self.baseline_runner_config = baseline_runner_config self.current_runner_config = current_runner_config + self.unit_timeout_secs = unit_timeout_secs + # Wall-clock per (unit, variant, rep). A run that overruns its pipeline budget is only + # actionable if it says WHERE the time went: a uniform slowdown across every unit points + # at the environment (CPU pinning, AV, virtualisation), whereas time concentrated in the + # connection-heavy units points at the workload or the server. + self.timings = [] def list_units(self): cmd = ["dotnet", os.path.join(self.current_dir, self.assembly)] @@ -189,8 +297,20 @@ def _run_one(self, variant, exe_dir, unit, rep, agg_dir): runner_config = (self.baseline_runner_config if variant == "baseline" else self.current_runner_config) env_overrides = {"RUNNER_CONFIG": runner_config} if runner_config else None - rc = run_unit_process(exe_dir, self.assembly, unit, cwd, self.cpus, log_path, - env_overrides=env_overrides) + rc, elapsed, affinity_note, timed_out = run_unit_process( + exe_dir, self.assembly, unit, cwd, self.cpus, log_path, + env_overrides=env_overrides, timeout_secs=self.unit_timeout_secs) + self.timings.append({ + "unit": unit, "variant": variant, "rep": rep, + "seconds": round(elapsed, 1), "timedOut": timed_out, + }) + print(f" {variant:<8} {_fmt_duration(elapsed):>9} [{affinity_note}]", flush=True) + if timed_out: + _tail(log_path) + raise RuntimeError( + f"benchmark unit '{unit}' ({variant}, rep {rep}) exceeded the per-unit timeout " + f"of {self.unit_timeout_secs}s and was killed. The tail of its log is above; the " + f"full log is at {log_path}.") if rc != 0: _tail(log_path) raise RuntimeError(f"benchmark unit '{unit}' ({variant}, rep {rep}) failed (exit {rc}).") @@ -217,6 +337,16 @@ def interleave(self, units, rep, baseline_agg, current_agg): return unit_types +def _fmt_duration(secs): + """Render a duration compactly, e.g. '4.2s', '3m12s', '1h04m'.""" + secs = int(round(secs)) + if secs < 60: + return f"{secs}s" + if secs < 3600: + return f"{secs // 60}m{secs % 60:02d}s" + return f"{secs // 3600}h{(secs % 3600) // 60:02d}m" + + def _tail(path, n=40): try: with open(path, "r", encoding="utf-8", errors="replace") as fh: @@ -350,6 +480,41 @@ def render_markdown(entries, confirmed, unconfirmed, baseline_version, threshold return "\n".join(lines) +def write_timing_report(runner, results_dir): + """Persist and print per-unit wall-clock, slowest first. + + This is the first thing to look at when a run overruns its pipeline budget: it separates a + uniform environmental slowdown from time concentrated in particular units, which the raw + benchmark numbers cannot show (they report per-operation time, not the setup, pool warm-up + and process startup that surround it). + """ + if not runner.timings: + return + diag_dir = os.path.join(results_dir, "diagnostics") + os.makedirs(diag_dir, exist_ok=True) + total = sum(t["seconds"] for t in runner.timings) + + per_unit = {} + for t in runner.timings: + per_unit.setdefault(t["unit"], 0.0) + per_unit[t["unit"]] += t["seconds"] + + with open(os.path.join(diag_dir, "unit-timings.json"), "w", encoding="utf-8") as fh: + json.dump({"totalSeconds": round(total, 1), + "perUnitSeconds": {k: round(v, 1) for k, v in per_unit.items()}, + "runs": runner.timings}, fh, indent=2) + + print("") + print("Benchmark wall-clock by unit (slowest first):") + print(f" {'unit':<42}{'total':>10}{'runs':>6}{'share':>8}") + for unit, secs in sorted(per_unit.items(), key=lambda kv: kv[1], reverse=True): + runs = sum(1 for t in runner.timings if t["unit"] == unit) + share = (secs / total * 100.0) if total else 0.0 + print(f" {unit:<42}{_fmt_duration(secs):>10}{runs:>6}{share:>7.1f}%") + print(f" {'TOTAL (benchmark subprocesses only)':<42}{_fmt_duration(total):>10}") + print("") + + # -------------------------------------------------------------------------------------------------- # Entry point. # -------------------------------------------------------------------------------------------------- @@ -378,6 +543,11 @@ def main(argv=None): help="Override RUNNER_CONFIG for current-variant subprocesses only.") parser.add_argument("--client-cpus", default=os.environ.get("PERF_CLIENT_CPUS", ""), help="CPU set to pin the benchmark client to, e.g. '16-31'.") + parser.add_argument("--unit-timeout-mins", type=int, + default=int(os.environ.get("PERF_UNIT_TIMEOUT_MINS", "45")), + help="Per-unit wall-clock backstop in minutes (0 disables). A wedged " + "unit would otherwise consume the whole pipeline budget and be " + "killed by the agent with no indication of which unit hung.") parser.add_argument("--fail-on-regression", action="store_true", help="Exit non-zero if any CONFIRMED regression is detected.") args = parser.parse_args(argv) @@ -401,7 +571,13 @@ def main(argv=None): os.path.abspath(args.current_exe_dir), args.assembly, work_dir, cpus, baseline_runner_config=args.baseline_runner_config, - current_runner_config=args.current_runner_config) + current_runner_config=args.current_runner_config, + unit_timeout_secs=max(0, args.unit_timeout_mins) * 60) + if args.unit_timeout_mins > 0: + print(f"Per-unit timeout: {args.unit_timeout_mins} minute(s).") + else: + print("Per-unit timeout disabled; a wedged unit will consume the whole job budget.", + file=sys.stderr) units = runner.list_units() if not units: @@ -409,8 +585,13 @@ def main(argv=None): return 1 print(f"Enabled units ({len(units)}): {', '.join(units)}") - entries, confirmed, unconfirmed = orchestrate( - runner, units, results_dir, args.threshold, args.reps) + try: + entries, confirmed, unconfirmed = orchestrate( + runner, units, results_dir, args.threshold, args.reps) + finally: + # Emit timings even when a unit failed or timed out -- that is exactly the case where + # knowing how long each unit took is most useful. + write_timing_report(runner, results_dir) # Outputs. comparison_dir = os.path.join(results_dir, "comparison") From 057604e8368c01c39b6d4047ceb4337231899d9b Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Wed, 26 Aug 2026 11:45:26 -0700 Subject: [PATCH 31/34] Stop the perf run before the pipeline kills it The Windows perf job runs until the pipeline's 180-minute job timeout while the same suite finishes on Linux in under an hour. That kill is uncatchable: no finally block runs, so every diagnostic produced up to that point is discarded and each run tells us only that it was too slow, never where the time went. Bound the run from inside so the diagnostics survive: - Add --total-budget-mins (PERF_TOTAL_BUDGET_MINS, default 150). Once it passes, stop starting units, record the ones not reached, write the timing breakdown, and fail naming the shortfall. 150 sits under the 180-minute pipeline default with margin for result collection. - Clamp each unit's timeout to the remaining budget, so a single slow unit cannot overshoot the deadline and forfeit the report. - Lower the per-unit default from 45 to 15 minutes. The Linux run completes every unit in low single-digit minutes, so 45 was far too loose to fail fast while still leaving generous headroom over a healthy unit. - Report skipped units in both the console summary and unit-timings.json. - Guard the timing report in the finally block: an exception there would replace the failure that sent us into it, losing the real error. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- eng/pipelines/perf/scripts/interleave_perf.py | 95 ++++++++++++++++--- 1 file changed, 83 insertions(+), 12 deletions(-) diff --git a/eng/pipelines/perf/scripts/interleave_perf.py b/eng/pipelines/perf/scripts/interleave_perf.py index 5c5739168d..2d65d831d9 100644 --- a/eng/pipelines/perf/scripts/interleave_perf.py +++ b/eng/pipelines/perf/scripts/interleave_perf.py @@ -262,7 +262,7 @@ class Runner: def __init__(self, baseline_dir, current_dir, assembly, work_dir, cpus, baseline_runner_config=None, current_runner_config=None, - unit_timeout_secs=0): + unit_timeout_secs=0, deadline=None): self.baseline_dir = baseline_dir self.current_dir = current_dir self.assembly = assembly @@ -274,11 +274,24 @@ def __init__(self, baseline_dir, current_dir, assembly, work_dir, cpus, self.baseline_runner_config = baseline_runner_config self.current_runner_config = current_runner_config self.unit_timeout_secs = unit_timeout_secs + # Absolute monotonic deadline (or None). The surrounding pipeline kills the whole job at + # its own timeout, and that kill is not catchable -- no finally block runs and every + # diagnostic produced so far is lost. Stopping ourselves before that point is what makes + # an overrun diagnosable at all, so the deadline is enforced here rather than left to the + # pipeline. + self.deadline = deadline # Wall-clock per (unit, variant, rep). A run that overruns its pipeline budget is only # actionable if it says WHERE the time went: a uniform slowdown across every unit points # at the environment (CPU pinning, AV, virtualisation), whereas time concentrated in the # connection-heavy units points at the workload or the server. self.timings = [] + self.skipped = [] + + def remaining_secs(self): + """Seconds left before the self-imposed deadline, or None when no deadline is set.""" + if self.deadline is None: + return None + return self.deadline - time.monotonic() def list_units(self): cmd = ["dotnet", os.path.join(self.current_dir, self.assembly)] @@ -297,9 +310,16 @@ def _run_one(self, variant, exe_dir, unit, rep, agg_dir): runner_config = (self.baseline_runner_config if variant == "baseline" else self.current_runner_config) env_overrides = {"RUNNER_CONFIG": runner_config} if runner_config else None + # Never let a single unit run past the deadline: the pipeline's own kill is uncatchable, + # so overshooting here costs the entire diagnostic output rather than just this unit. + timeout = self.unit_timeout_secs + remaining = self.remaining_secs() + if remaining is not None: + remaining = max(1, int(remaining)) + timeout = remaining if timeout <= 0 else min(timeout, remaining) rc, elapsed, affinity_note, timed_out = run_unit_process( exe_dir, self.assembly, unit, cwd, self.cpus, log_path, - env_overrides=env_overrides, timeout_secs=self.unit_timeout_secs) + env_overrides=env_overrides, timeout_secs=timeout) self.timings.append({ "unit": unit, "variant": variant, "rep": rep, "seconds": round(elapsed, 1), "timedOut": timed_out, @@ -308,9 +328,9 @@ def _run_one(self, variant, exe_dir, unit, rep, agg_dir): if timed_out: _tail(log_path) raise RuntimeError( - f"benchmark unit '{unit}' ({variant}, rep {rep}) exceeded the per-unit timeout " - f"of {self.unit_timeout_secs}s and was killed. The tail of its log is above; the " - f"full log is at {log_path}.") + f"benchmark unit '{unit}' ({variant}, rep {rep}) exceeded its {timeout}s " + f"time limit and was killed. The tail of its log is above; the full log is at " + f"{log_path}.") if rc != 0: _tail(log_path) raise RuntimeError(f"benchmark unit '{unit}' ({variant}, rep {rep}) failed (exit {rc}).") @@ -326,10 +346,24 @@ def interleave(self, units, rep, baseline_agg, current_agg): """Run each unit baseline-then-candidate; aggregate reports per variant. Returns a dict mapping unit name -> set of report Type names it produced. + + Stops early once the deadline passes, recording the units it did not reach. A partial + run that names what it skipped is diagnosable; a job killed by the pipeline is not. """ unit_types = {} - for unit in units: - print(f"[rep {rep}] {unit}: baseline -> candidate", flush=True) + for i, unit in enumerate(units): + remaining = self.remaining_secs() + if remaining is not None and remaining <= 0: + not_run = list(units[i:]) + self.skipped.extend((unit_name, rep) for unit_name in not_run) + print(f"\nSTOPPING EARLY: the run budget is exhausted with {len(not_run)} of " + f"{len(units)} unit(s) left in rep {rep}: {', '.join(not_run)}", + file=sys.stderr) + print("Emitting partial results so the timing breakdown survives; the pipeline " + "would otherwise kill this job and discard it.", file=sys.stderr) + break + budget = f" ({_fmt_duration(remaining)} of budget left)" if remaining is not None else "" + print(f"[rep {rep}] {unit}: baseline -> candidate{budget}", flush=True) # Interleaved: measure baseline and candidate for this unit back-to-back. self._run_one("baseline", self.baseline_dir, unit, rep, baseline_agg) types = self._run_one("current", self.current_dir, unit, rep, current_agg) @@ -490,6 +524,7 @@ def write_timing_report(runner, results_dir): """ if not runner.timings: return + skipped = list(getattr(runner, "skipped", [])) diag_dir = os.path.join(results_dir, "diagnostics") os.makedirs(diag_dir, exist_ok=True) total = sum(t["seconds"] for t in runner.timings) @@ -502,6 +537,7 @@ def write_timing_report(runner, results_dir): with open(os.path.join(diag_dir, "unit-timings.json"), "w", encoding="utf-8") as fh: json.dump({"totalSeconds": round(total, 1), "perUnitSeconds": {k: round(v, 1) for k, v in per_unit.items()}, + "skipped": [{"unit": u, "rep": r} for u, r in skipped], "runs": runner.timings}, fh, indent=2) print("") @@ -512,6 +548,11 @@ def write_timing_report(runner, results_dir): share = (secs / total * 100.0) if total else 0.0 print(f" {unit:<42}{_fmt_duration(secs):>10}{runs:>6}{share:>7.1f}%") print(f" {'TOTAL (benchmark subprocesses only)':<42}{_fmt_duration(total):>10}") + if skipped: + print("") + print(f"NOT RUN ({len(skipped)}, budget exhausted):") + for unit, rep in skipped: + print(f" {unit} (rep {rep})") print("") @@ -544,10 +585,19 @@ def main(argv=None): parser.add_argument("--client-cpus", default=os.environ.get("PERF_CLIENT_CPUS", ""), help="CPU set to pin the benchmark client to, e.g. '16-31'.") parser.add_argument("--unit-timeout-mins", type=int, - default=int(os.environ.get("PERF_UNIT_TIMEOUT_MINS", "45")), + default=int(os.environ.get("PERF_UNIT_TIMEOUT_MINS", "15")), help="Per-unit wall-clock backstop in minutes (0 disables). A wedged " "unit would otherwise consume the whole pipeline budget and be " - "killed by the agent with no indication of which unit hung.") + "killed by the agent with no indication of which unit hung. The " + "default leaves generous headroom over a healthy unit, which " + "completes in low single-digit minutes.") + parser.add_argument("--total-budget-mins", type=int, + default=int(os.environ.get("PERF_TOTAL_BUDGET_MINS", "150")), + help="Stop starting new units after this many minutes (0 disables). Set " + "below the pipeline's own job timeout: that kill is uncatchable, so " + "without this the run loses every diagnostic it has produced. The " + "default sits under the 180-minute pipeline default with margin for " + "result collection.") parser.add_argument("--fail-on-regression", action="store_true", help="Exit non-zero if any CONFIRMED regression is detected.") args = parser.parse_args(argv) @@ -567,17 +617,27 @@ def main(argv=None): else: print("PERF_CLIENT_CPUS not set; running without CPU pinning.", file=sys.stderr) + deadline = (time.monotonic() + args.total_budget_mins * 60 + if args.total_budget_mins > 0 else None) + runner = Runner(os.path.abspath(args.baseline_exe_dir), os.path.abspath(args.current_exe_dir), args.assembly, work_dir, cpus, baseline_runner_config=args.baseline_runner_config, current_runner_config=args.current_runner_config, - unit_timeout_secs=max(0, args.unit_timeout_mins) * 60) + unit_timeout_secs=max(0, args.unit_timeout_mins) * 60, + deadline=deadline) if args.unit_timeout_mins > 0: print(f"Per-unit timeout: {args.unit_timeout_mins} minute(s).") else: print("Per-unit timeout disabled; a wedged unit will consume the whole job budget.", file=sys.stderr) + if deadline is not None: + print(f"Run budget: {args.total_budget_mins} minute(s); the run stops early rather than " + f"letting the pipeline kill it and discard the diagnostics.") + else: + print("Run budget disabled; a pipeline timeout will discard all diagnostics.", + file=sys.stderr) units = runner.list_units() if not units: @@ -590,8 +650,12 @@ def main(argv=None): runner, units, results_dir, args.threshold, args.reps) finally: # Emit timings even when a unit failed or timed out -- that is exactly the case where - # knowing how long each unit took is most useful. - write_timing_report(runner, results_dir) + # knowing how long each unit took is most useful. Guarded: an exception raised here + # would replace the real failure that sent us into this block. + try: + write_timing_report(runner, results_dir) + except Exception as exc: # noqa: BLE001 + print(f"WARNING: could not write the timing report: {exc}", file=sys.stderr) # Outputs. comparison_dir = os.path.join(results_dir, "comparison") @@ -615,6 +679,13 @@ def main(argv=None): print(f"Interleaved comparison complete: {len(confirmed)} confirmed regression(s), " f"{len(unconfirmed)} unconfirmed.") + if runner.skipped: + print(f"ERROR: the run budget ({args.total_budget_mins} min) was exhausted before " + f"{len(runner.skipped)} unit run(s) could execute, so this comparison is " + f"incomplete. See the wall-clock breakdown above for where the time went.", + file=sys.stderr) + return 1 + if args.fail_on_regression and confirmed: print("Confirmed regressions detected; failing as requested.", file=sys.stderr) for e in confirmed: From b9f2b7ddd57394eaa24d82c9c7e7b1f8af7e639d Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Wed, 26 Aug 2026 13:43:56 -0700 Subject: [PATCH 32/34] Add a connection-open latency probe to the perf run The instrumented Windows run localised the overrun: the first unit, SqlConnection, consumed the entire 15-minute limit on its own, holding a steady ~153 ms per SqlConnection.Open against a SQL Server pinned to the other half of the same VM. The tight spread (152.6-156.1 ms) makes this a fixed per-operation cost rather than contention or a hang, and at that rate the unit needs roughly 34 minutes for its 8 cases. That cost is invisible in the normal output. The benchmark suite reports an aggregate mean per case after many minutes of work, so a host where every physical connect carries a fixed penalty is indistinguishable from a broad driver regression, which is what sent the earlier investigation toward ephemeral ports and CPU pinning. Add PERF_CONNECT_PROBE to the perf host: it times unpooled and pooled opens through the same driver, config and connection string the benchmarks use and prints min/p50/p90/max for each. Splitting the two localises the cost, since pooled opens skip the physical connect. The orchestrator runs it once before the first unit, so every run records it on both platforms and the numbers are directly comparable. It is a diagnostic, not a gate: a failure is reported and the run continues. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- eng/pipelines/perf/scripts/interleave_perf.py | 33 ++++++++ .../tests/PerformanceTests/Program.cs | 83 +++++++++++++++++++ 2 files changed, 116 insertions(+) diff --git a/eng/pipelines/perf/scripts/interleave_perf.py b/eng/pipelines/perf/scripts/interleave_perf.py index 2d65d831d9..0c920e1a2b 100644 --- a/eng/pipelines/perf/scripts/interleave_perf.py +++ b/eng/pipelines/perf/scripts/interleave_perf.py @@ -556,6 +556,37 @@ def write_timing_report(runner, results_dir): print("") +def run_connect_probe(runner, results_dir): + """Measure raw connection-open latency once, before any benchmark runs. + + A host where every physical connect carries a fixed extra cost looks identical to a broad + driver regression in the benchmark output, and costs many minutes per unit to observe. This + probe answers that in seconds, and because it runs on both platforms it makes the Windows and + Linux numbers directly comparable. Never fatal: it is a diagnostic, not a gate. + """ + diag_dir = os.path.join(results_dir, "diagnostics") + os.makedirs(diag_dir, exist_ok=True) + out_path = os.path.join(diag_dir, "connect-probe.txt") + env = dict(os.environ) + env["PERF_CONNECT_PROBE"] = "1" + env.pop("PERF_BENCHMARK", None) + env.pop("PERF_LIST_BENCHMARKS", None) + if runner.current_runner_config: + env["RUNNER_CONFIG"] = runner.current_runner_config + cmd = ["dotnet", os.path.join(runner.current_dir, runner.assembly)] + try: + out = subprocess.run(cmd, cwd=runner.work_dir, env=env, text=True, timeout=300, + stdout=subprocess.PIPE, stderr=subprocess.STDOUT).stdout + except Exception as exc: # noqa: BLE001 + print(f"WARNING: connection-open probe did not run: {exc}", file=sys.stderr) + return + with open(out_path, "w", encoding="utf-8") as fh: + fh.write(out) + print(out.rstrip()) + print(f" (saved to {out_path})") + print("") + + # -------------------------------------------------------------------------------------------------- # Entry point. # -------------------------------------------------------------------------------------------------- @@ -645,6 +676,8 @@ def main(argv=None): return 1 print(f"Enabled units ({len(units)}): {', '.join(units)}") + run_connect_probe(runner, results_dir) + try: entries, confirmed, unconfirmed = orchestrate( runner, units, results_dir, args.threshold, args.reps) diff --git a/src/Microsoft.Data.SqlClient/tests/PerformanceTests/Program.cs b/src/Microsoft.Data.SqlClient/tests/PerformanceTests/Program.cs index 8e305efddd..73180a366b 100644 --- a/src/Microsoft.Data.SqlClient/tests/PerformanceTests/Program.cs +++ b/src/Microsoft.Data.SqlClient/tests/PerformanceTests/Program.cs @@ -86,6 +86,12 @@ public Program() return; } + if (Environment.GetEnvironmentVariable("PERF_CONNECT_PROBE") != null) + { + RunConnectProbe(); + return; + } + string only = Environment.GetEnvironmentVariable("PERF_BENCHMARK"); IEnumerable toRun = enabled; if (!string.IsNullOrWhiteSpace(only)) @@ -129,8 +135,85 @@ private bool IsEnabled(BenchmarkUnit unit) /// private static bool IsHarnessControlled() => Environment.GetEnvironmentVariable("PERF_LIST_BENCHMARKS") != null || + Environment.GetEnvironmentVariable("PERF_CONNECT_PROBE") != null || !string.IsNullOrWhiteSpace(Environment.GetEnvironmentVariable("PERF_BENCHMARK")); + /// + /// Measures raw latency and prints a percentile summary. + /// + /// + /// This exists to make one specific question cheap to answer: is a slow perf run slow + /// because of the driver change under test, or because opening a connection on this host is + /// slow to begin with? The benchmark suite cannot answer it -- a single unit takes many + /// minutes and reports only an aggregate mean, so an environment where every physical + /// connect costs an extra fixed amount looks exactly like a broad regression. + /// + /// It deliberately runs through the same driver, config and connection string as the + /// benchmarks, so pooled-vs-unpooled and platform-vs-platform numbers are directly + /// comparable. Percentiles rather than a mean, because a fixed per-connect penalty (a + /// handshake, a lookup that has to time out) and occasional interference produce very + /// different distributions and the mean alone cannot tell them apart. + /// + private void RunConnectProbe() + { + const int UnpooledOpens = 25; + const int PooledOpens = 500; + + string baseCs = _config.ConnectionString; + Console.WriteLine("Connection-open latency probe"); + Console.WriteLine($" Runtime : {System.Runtime.InteropServices.RuntimeInformation.FrameworkDescription}"); + Console.WriteLine($" OS : {System.Runtime.InteropServices.RuntimeInformation.OSDescription}"); + Console.WriteLine($" Managed SNI switch: {_config.UseManagedSniOnWindows}"); + Console.WriteLine(); + + // Unpooled first: every open is a real TCP connect + TDS login, which is what a fixed + // per-connection penalty shows up in. Pooled opens mostly skip that work, so a large + // gap between the two localises the cost to the physical connect. + Measure("unpooled (physical connect + login)", $"{baseCs};Pooling=False", UnpooledOpens); + Measure("pooled (reuse from pool)", $"{baseCs};Pooling=True", PooledOpens); + } + + private static void Measure(string label, string connectionString, int count) + { + var samples = new List(count); + var sw = new System.Diagnostics.Stopwatch(); + + // One untimed open so first-call costs (JIT, TLS material, pool group creation) are not + // attributed to the steady-state distribution being measured. + try + { + using var warm = new SqlConnection(connectionString); + warm.Open(); + } + catch (Exception ex) + { + Console.WriteLine($" {label}: FAILED to connect: {ex.Message}"); + return; + } + + for (int i = 0; i < count; i++) + { + using var connection = new SqlConnection(connectionString); + sw.Restart(); + connection.Open(); + sw.Stop(); + samples.Add(sw.Elapsed.TotalMilliseconds); + } + + samples.Sort(); + Console.WriteLine($" {label} (n={count})"); + Console.WriteLine($" min {samples[0]:F3} ms | p50 {Percentile(samples, 50):F3} ms | " + + $"p90 {Percentile(samples, 90):F3} ms | max {samples[samples.Count - 1]:F3} ms"); + SqlConnection.ClearAllPools(); + } + + private static double Percentile(List sorted, int percentile) + { + // Nearest-rank on an already-sorted list. + int rank = (int)Math.Ceiling(percentile / 100.0 * sorted.Count); + return sorted[Math.Min(Math.Max(rank - 1, 0), sorted.Count - 1)]; + } + private void SetupConfigurations() { // If the config file specifies to use managed SNI on Windows, From 9f7c7d01b91915232c7a9908f212e7acfe2e69b9 Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Wed, 26 Aug 2026 14:29:57 -0700 Subject: [PATCH 33/34] Split the connect probe by SNI implementation and raw socket The probe answered its first question: on Windows a pooled open costs 0.001 ms while an unpooled open costs 155.9 ms at p50. The pool and the driver's reuse path are fine; the entire cost is in establishing a new physical connection to a SQL Server sitting on the other half of the same VM. That still spans three candidates: the socket connect itself, the work the driver layers on top of it (pre-login, TLS handshake, TDS login), and the SNI implementation underneath both. Native SNI is the last remaining difference between Windows and Linux on this path, and the switch latches at first open, so the two cannot be compared within a single process. Measure a bare socket connect to the same endpoint, which bounds how much of the 156 ms the network can account for, and on Windows run the probe once per SNI implementation. Together those three numbers attribute the cost to the network, the handshake, or native SNI without another guess. PERF_MANAGED_SNI overrides the config switch so the orchestrator can drive the two passes. Endpoint parsing and socket failures degrade to a note rather than failing the probe. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- eng/pipelines/perf/scripts/interleave_perf.py | 47 +++++++--- .../tests/PerformanceTests/Program.cs | 93 ++++++++++++++++++- 2 files changed, 122 insertions(+), 18 deletions(-) diff --git a/eng/pipelines/perf/scripts/interleave_perf.py b/eng/pipelines/perf/scripts/interleave_perf.py index 0c920e1a2b..790cfeb80f 100644 --- a/eng/pipelines/perf/scripts/interleave_perf.py +++ b/eng/pipelines/perf/scripts/interleave_perf.py @@ -563,26 +563,45 @@ def run_connect_probe(runner, results_dir): driver regression in the benchmark output, and costs many minutes per unit to observe. This probe answers that in seconds, and because it runs on both platforms it makes the Windows and Linux numbers directly comparable. Never fatal: it is a diagnostic, not a gate. + + On Windows the probe runs twice, once per SNI implementation. Native SNI is the only remaining + difference from Linux on the connect path, and the switch is latched at first open, so the two + cannot be compared inside one process. """ diag_dir = os.path.join(results_dir, "diagnostics") os.makedirs(diag_dir, exist_ok=True) out_path = os.path.join(diag_dir, "connect-probe.txt") - env = dict(os.environ) - env["PERF_CONNECT_PROBE"] = "1" - env.pop("PERF_BENCHMARK", None) - env.pop("PERF_LIST_BENCHMARKS", None) - if runner.current_runner_config: - env["RUNNER_CONFIG"] = runner.current_runner_config - cmd = ["dotnet", os.path.join(runner.current_dir, runner.assembly)] - try: - out = subprocess.run(cmd, cwd=runner.work_dir, env=env, text=True, timeout=300, - stdout=subprocess.PIPE, stderr=subprocess.STDOUT).stdout - except Exception as exc: # noqa: BLE001 - print(f"WARNING: connection-open probe did not run: {exc}", file=sys.stderr) + + passes = [("configured SNI", None)] + if os.name == "nt": + passes = [("native SNI", "0"), ("managed SNI", "1")] + + captured = [] + for label, managed_sni in passes: + env = dict(os.environ) + env["PERF_CONNECT_PROBE"] = "1" + env.pop("PERF_BENCHMARK", None) + env.pop("PERF_LIST_BENCHMARKS", None) + if managed_sni is not None: + env["PERF_MANAGED_SNI"] = managed_sni + if runner.current_runner_config: + env["RUNNER_CONFIG"] = runner.current_runner_config + cmd = ["dotnet", os.path.join(runner.current_dir, runner.assembly)] + try: + out = subprocess.run(cmd, cwd=runner.work_dir, env=env, text=True, timeout=300, + stdout=subprocess.PIPE, stderr=subprocess.STDOUT).stdout + except Exception as exc: # noqa: BLE001 + print(f"WARNING: connection-open probe ({label}) did not run: {exc}", file=sys.stderr) + continue + captured.append(f"===== {label} =====\n{out.rstrip()}") + + if not captured: return + + report = "\n\n".join(captured) with open(out_path, "w", encoding="utf-8") as fh: - fh.write(out) - print(out.rstrip()) + fh.write(report + "\n") + print(report) print(f" (saved to {out_path})") print("") diff --git a/src/Microsoft.Data.SqlClient/tests/PerformanceTests/Program.cs b/src/Microsoft.Data.SqlClient/tests/PerformanceTests/Program.cs index 73180a366b..e550bb137a 100644 --- a/src/Microsoft.Data.SqlClient/tests/PerformanceTests/Program.cs +++ b/src/Microsoft.Data.SqlClient/tests/PerformanceTests/Program.cs @@ -5,6 +5,7 @@ using System; using System.Collections.Generic; using System.Linq; +using System.Net.Sockets; using BenchmarkDotNet.Running; namespace Microsoft.Data.SqlClient.PerformanceTests @@ -13,6 +14,12 @@ public class Program { private readonly Config _config; + /// + /// The SNI implementation actually in effect, after any PERF_MANAGED_SNI override of + /// the config value. Resolved in . + /// + private bool _useManagedSni; + /// /// A single benchmark "unit": the stable selector name used by the perf pipeline, the /// accessor for its per-benchmark in the runner config, and the @@ -158,21 +165,80 @@ private void RunConnectProbe() { const int UnpooledOpens = 25; const int PooledOpens = 500; + const int RawTcpConnects = 25; string baseCs = _config.ConnectionString; Console.WriteLine("Connection-open latency probe"); Console.WriteLine($" Runtime : {System.Runtime.InteropServices.RuntimeInformation.FrameworkDescription}"); Console.WriteLine($" OS : {System.Runtime.InteropServices.RuntimeInformation.OSDescription}"); - Console.WriteLine($" Managed SNI switch: {_config.UseManagedSniOnWindows}"); + Console.WriteLine($" Managed SNI switch: {_useManagedSni}"); Console.WriteLine(); - // Unpooled first: every open is a real TCP connect + TDS login, which is what a fixed + // A bare socket connect to the same endpoint, with no driver involved. This is the floor + // the other numbers are measured against: it separates a slow network path from cost the + // driver adds on top of it (pre-login, TLS handshake, TDS login). + MeasureRawTcp(baseCs, RawTcpConnects); + + // Unpooled next: every open is a real TCP connect + TDS login, which is what a fixed // per-connection penalty shows up in. Pooled opens mostly skip that work, so a large // gap between the two localises the cost to the physical connect. Measure("unpooled (physical connect + login)", $"{baseCs};Pooling=False", UnpooledOpens); Measure("pooled (reuse from pool)", $"{baseCs};Pooling=True", PooledOpens); } + /// + /// Times bare TCP connects to the server endpoint, bypassing the driver entirely. + /// + private static void MeasureRawTcp(string connectionString, int count) + { + const string Label = "raw TCP (socket connect only, no driver)"; + + string host; + int port; + try + { + // DataSource is "tcp:," in the perf connection strings; tolerate the + // prefix and port being absent rather than failing the whole probe. + string dataSource = new SqlConnectionStringBuilder(connectionString).DataSource; + if (dataSource.StartsWith("tcp:", StringComparison.OrdinalIgnoreCase)) + { + dataSource = dataSource.Substring(4); + } + + int comma = dataSource.IndexOf(','); + host = comma >= 0 ? dataSource.Substring(0, comma) : dataSource; + port = comma >= 0 && int.TryParse(dataSource.Substring(comma + 1), out int parsed) ? parsed : 1433; + } + catch (Exception ex) + { + Console.WriteLine($" {Label}: SKIPPED, could not parse endpoint: {ex.Message}"); + Console.WriteLine(); + return; + } + + var samples = new List(count); + var sw = new System.Diagnostics.Stopwatch(); + for (int i = 0; i < count; i++) + { + try + { + using var socket = new Socket(AddressFamily.InterNetwork, SocketType.Stream, ProtocolType.Tcp); + sw.Restart(); + socket.Connect(host, port); + sw.Stop(); + samples.Add(sw.Elapsed.TotalMilliseconds); + } + catch (Exception ex) + { + Console.WriteLine($" {Label}: FAILED to connect to {host}:{port}: {ex.Message}"); + Console.WriteLine(); + return; + } + } + + Report(Label, samples, count); + } + private static void Measure(string label, string connectionString, int count) { var samples = new List(count); @@ -188,6 +254,7 @@ private static void Measure(string label, string connectionString, int count) catch (Exception ex) { Console.WriteLine($" {label}: FAILED to connect: {ex.Message}"); + Console.WriteLine(); return; } @@ -200,11 +267,17 @@ private static void Measure(string label, string connectionString, int count) samples.Add(sw.Elapsed.TotalMilliseconds); } + Report(label, samples, count); + SqlConnection.ClearAllPools(); + } + + private static void Report(string label, List samples, int count) + { samples.Sort(); Console.WriteLine($" {label} (n={count})"); Console.WriteLine($" min {samples[0]:F3} ms | p50 {Percentile(samples, 50):F3} ms | " + $"p90 {Percentile(samples, 90):F3} ms | max {samples[samples.Count - 1]:F3} ms"); - SqlConnection.ClearAllPools(); + Console.WriteLine(); } private static double Percentile(List sorted, int percentile) @@ -218,7 +291,19 @@ private void SetupConfigurations() { // If the config file specifies to use managed SNI on Windows, // enable the appropriate AppContext switch to use the managed SNI implementation. - if (_config.UseManagedSniOnWindows) + // + // PERF_MANAGED_SNI overrides the config value. The switch is latched the first time a + // connection is opened, so the two SNI implementations cannot be compared inside one + // process; the override lets the harness run the probe twice, once each way, to + // attribute connect cost to native SNI or rule it out. + _useManagedSni = _config.UseManagedSniOnWindows; + string sniOverride = Environment.GetEnvironmentVariable("PERF_MANAGED_SNI"); + if (!string.IsNullOrWhiteSpace(sniOverride)) + { + _useManagedSni = sniOverride is "1" or "true" or "True" or "TRUE"; + } + + if (_useManagedSni) { AppContext.SetSwitch("Switch.Microsoft.Data.SqlClient.UseManagedNetworkingOnWindows", true); } From 3f76729a8801cfe791f09e872c3f2bfeb873b8fa Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Wed, 26 Aug 2026 15:14:23 -0700 Subject: [PATCH 34/34] Measure the TLS handshake separately in the connect probe The two-pass probe ruled out the suspect it was built to test. Native and managed SNI cost the same per connect on Windows, 158.3 ms and 157.6 ms at p50, so the SNI implementation is not responsible. The raw socket connect came back at 0.06 ms, so the network is not either. The cost sits entirely in the handshake sequence the two implementations share: pre-login, TLS, login. Split that sequence. A TLS handshake straight onto the SQL port has the same shape as TDS 8.0, so the timing needs no TDS framing to reproduce, and the login packet is TLS-protected even when Encrypt=False, so this cost is paid on every connect regardless of the encryption setting. Timing it against the 158 ms full open says whether the handshake or the TDS login owns the time. Run it twice, differing only in revocation checking. A revocation lookup that has to reach the network or wait out a timeout is a classic fixed per-handshake penalty and fits the near-constant cost being observed, so it is worth answering directly rather than by elimination. The socket connect is excluded from the handshake timing and certificate validation always succeeds, so the number is handshake work rather than trust decisions. A server that will not take TLS directly on the port is reported and the remaining measurements still run. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../tests/PerformanceTests/Program.cs | 88 ++++++++++++++++--- 1 file changed, 75 insertions(+), 13 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/tests/PerformanceTests/Program.cs b/src/Microsoft.Data.SqlClient/tests/PerformanceTests/Program.cs index e550bb137a..c32718abc8 100644 --- a/src/Microsoft.Data.SqlClient/tests/PerformanceTests/Program.cs +++ b/src/Microsoft.Data.SqlClient/tests/PerformanceTests/Program.cs @@ -5,7 +5,9 @@ using System; using System.Collections.Generic; using System.Linq; +using System.Net.Security; using System.Net.Sockets; +using System.Security.Authentication; using BenchmarkDotNet.Running; namespace Microsoft.Data.SqlClient.PerformanceTests @@ -166,6 +168,7 @@ private void RunConnectProbe() const int UnpooledOpens = 25; const int PooledOpens = 500; const int RawTcpConnects = 25; + const int TlsHandshakes = 15; string baseCs = _config.ConnectionString; Console.WriteLine("Connection-open latency probe"); @@ -174,10 +177,27 @@ private void RunConnectProbe() Console.WriteLine($" Managed SNI switch: {_useManagedSni}"); Console.WriteLine(); - // A bare socket connect to the same endpoint, with no driver involved. This is the floor - // the other numbers are measured against: it separates a slow network path from cost the - // driver adds on top of it (pre-login, TLS handshake, TDS login). - MeasureRawTcp(baseCs, RawTcpConnects); + if (!TryParseEndpoint(baseCs, out string host, out int port, out string parseError)) + { + Console.WriteLine($" network probes SKIPPED, could not parse endpoint: {parseError}"); + Console.WriteLine(); + } + else + { + // A bare socket connect, with no driver involved. This is the floor the other numbers + // are measured against: it separates a slow network path from cost added on top of it. + MeasureRawTcp(host, port, RawTcpConnects); + + // A TLS handshake straight onto the SQL port (the TDS 8.0 shape, so no TDS framing to + // reproduce). The login packet is TLS-protected even when Encrypt=False, so this cost + // is paid on every connect regardless. It separates handshake cost from TDS login. + // + // Run twice, differing only in revocation checking: a revocation lookup that has to + // reach the network or time out is a classic fixed per-handshake penalty, and this + // says directly whether that is what is being paid here. + MeasureTlsHandshake("TLS (handshake only, no revocation check)", host, port, TlsHandshakes, false); + MeasureTlsHandshake("TLS (handshake only, revocation checked)", host, port, TlsHandshakes, true); + } // Unpooled next: every open is a real TCP connect + TDS login, which is what a fixed // per-connection penalty shows up in. Pooled opens mostly skip that work, so a large @@ -187,14 +207,13 @@ private void RunConnectProbe() } /// - /// Times bare TCP connects to the server endpoint, bypassing the driver entirely. + /// Extracts the server host and port from a connection string's DataSource. /// - private static void MeasureRawTcp(string connectionString, int count) + private static bool TryParseEndpoint(string connectionString, out string host, out int port, out string error) { - const string Label = "raw TCP (socket connect only, no driver)"; - - string host; - int port; + host = null; + port = 0; + error = null; try { // DataSource is "tcp:," in the perf connection strings; tolerate the @@ -208,13 +227,21 @@ private static void MeasureRawTcp(string connectionString, int count) int comma = dataSource.IndexOf(','); host = comma >= 0 ? dataSource.Substring(0, comma) : dataSource; port = comma >= 0 && int.TryParse(dataSource.Substring(comma + 1), out int parsed) ? parsed : 1433; + return !string.IsNullOrWhiteSpace(host); } catch (Exception ex) { - Console.WriteLine($" {Label}: SKIPPED, could not parse endpoint: {ex.Message}"); - Console.WriteLine(); - return; + error = ex.Message; + return false; } + } + + /// + /// Times bare TCP connects to the server endpoint, bypassing the driver entirely. + /// + private static void MeasureRawTcp(string host, int port, int count) + { + const string Label = "raw TCP (socket connect only, no driver)"; var samples = new List(count); var sw = new System.Diagnostics.Stopwatch(); @@ -239,6 +266,41 @@ private static void MeasureRawTcp(string connectionString, int count) Report(Label, samples, count); } + /// + /// Times only the TLS handshake against the SQL Server endpoint. The socket connect is + /// excluded from the measurement, and certificate validation always succeeds, so the number + /// is the handshake itself rather than trust decisions layered on it. + /// + private static void MeasureTlsHandshake(string label, string host, int port, int count, bool checkRevocation) + { + var samples = new List(count); + var sw = new System.Diagnostics.Stopwatch(); + for (int i = 0; i < count; i++) + { + try + { + using var tcp = new TcpClient(); + tcp.Connect(host, port); + using var ssl = new SslStream(tcp.GetStream(), leaveInnerStreamOpen: false, + (sender, cert, chain, errors) => true); + sw.Restart(); + ssl.AuthenticateAsClient(host, null, SslProtocols.None, checkRevocation); + sw.Stop(); + samples.Add(sw.Elapsed.TotalMilliseconds); + } + catch (Exception ex) + { + // A server that will not take TLS directly on the port is a valid outcome, not a + // probe failure: report it and carry on with the remaining measurements. + Console.WriteLine($" {label}: NOT MEASURED: {ex.Message}"); + Console.WriteLine(); + return; + } + } + + Report(label, samples, count); + } + private static void Measure(string label, string connectionString, int count) { var samples = new List(count);