[Experimental] Perf benchmark follow-ups (stacked on #4447)#4455
Closed
cheenamalhotra wants to merge 8 commits into
Closed
[Experimental] Perf benchmark follow-ups (stacked on #4447)#4455cheenamalhotra wants to merge 8 commits into
cheenamalhotra wants to merge 8 commits into
Conversation
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
- AsyncLargeDataReadRunner: seed VARBINARY(MAX) row via CRYPT_GEN_RANDOM T-SQL instead of allocating a client-side byte[] and shipping it over the wire, and add [Params] for ReadBufferBytes so we can observe how the client read buffer size interacts with payload size. - CancellationTokenReadAsyncRunner: move CancellationTokenSource allocation to [IterationSetup] and disposal to [IterationCleanup] so only the per-row ReadAsync(CancellationToken) overhead is measured. - ConnectionPoolStressRunner: scale the RapidFireOpenClose per-task iteration count with MaxPoolSize/Parallelism so total checkouts stay proportional to pool capacity, and drop the unused System.Collections.Generic import. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 62d8a5ff-8bd4-4780-9715-748ea0d950aa
- JsonVsVarcharReadRunner: use SqlDataReader.GetSqlJson(0).Value for the JSON case (sync and async) so the benchmark exercises the SqlJson accessor path implicated in #3499 instead of the shared GetString / GetFieldValueAsync<string> path used for VARCHAR. - ConnectionPoolStressRunner.BurstyTrafficPattern: spin up Parallelism concurrent tasks per burst instead of Parallelism/burstCount, so the configured Parallelism parameter actually drives observed concurrency. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 62d8a5ff-8bd4-4780-9715-748ea0d950aa
- ConnectionPoolStressRunner: expand MaxPoolSize [Params] to include 500 and 1000 so the pool bookkeeping is exercised under high capacity (per @paulmedynski's suggestion to include larger sizes). - ConnectionPoolStressRunner: scale MixedSyncAsyncContention iterations and MultiCommandReuse burst size with MaxPoolSize/Parallelism so the workload per checkout tracks pool capacity regardless of [Params] values (mirrors the earlier RapidFireOpenClose scaling change). - Move server GC configuration to the perf test csproj via <ServerGarbageCollection>true</ServerGarbageCollection> so it actually applies to the InProcessEmitToolchain host, and drop the equivalent BDN .WithEnvironmentVariable("COMPlus_gcServer", "1") line, which had no effect since COMPlus_gcServer is a startup-only runtime knob. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 62d8a5ff-8bd4-4780-9715-748ea0d950aa
Contributor
There was a problem hiding this comment.
Pull request overview
Follow-up updates to the Microsoft.Data.SqlClient.PerformanceTests benchmark suite (stacked on #4447), focusing on making pool-stress benchmarks scale better with parameter magnitudes and ensuring Server GC configuration actually applies when running in-process via InProcessEmitToolchain.
Changes:
- Configure Server/Concurrent GC at the perf test host process level via project properties, and remove the ineffective
COMPlus_gcServerjob environment variable. - Expand
ConnectionPoolStressRunner.MaxPoolSizeparameter values to include higher-capacity pool sizes. - Scale selected
ConnectionPoolStressRunnerinner-loop workloads based onMaxPoolSize(andParallelism) to keep benchmark pressure proportional across parameter combinations.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/Microsoft.Data.SqlClient/tests/PerformanceTests/Microsoft.Data.SqlClient.PerformanceTests.csproj | Enables Server/Concurrent GC via csproj properties so it applies to the in-process benchmark host. |
| src/Microsoft.Data.SqlClient/tests/PerformanceTests/Config/BenchmarkConfig.cs | Removes COMPlus_gcServer environment configuration and documents the host-level GC setting approach. |
| src/Microsoft.Data.SqlClient/tests/PerformanceTests/BenchmarkRunners/ConnectionPoolStressRunner.cs | Adds larger MaxPoolSize params and scales workloads in stress benchmarks to better match pool capacity. |
Comment on lines
33
to
+37
| /// Max pool size — controls how many physical connections the pool can hold. | ||
| /// When Parallelism exceeds this, tasks must wait for a free connection. | ||
| /// The larger values exercise pool bookkeeping under high capacity; SQL Server | ||
| /// itself must be able to accept that many concurrent connections for the | ||
| /// higher values to be meaningful. |
cheenamalhotra
changed the title
Member
Author
|
Not needed anymore. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-ups to #4447 for review comments that were deferred out of that PR. Stacked on top of
dev/cheena/perf-projectand marked draft; retarget tomainonce #4447 merges.Changes
ConnectionPoolStressRunner: expandMaxPoolSize[Params]to include500and1000so the pool bookkeeping is exercised under high capacity, per @paulmedynski's suggestion about including larger pool sizes. (20k / 100k intentionally left out — they approach SQL Server's max-connection ceiling and would dominate CI wall-clock; happy to add them behind an opt-in[Params]if we get a dedicated stress lane.)ConnectionPoolStressRunner: scaleMixedSyncAsyncContentioniterations andMultiCommandReusecommand-burst size withMaxPoolSize / Parallelism, addressing @paulmedynski's note that the hardcoded inner-loop values interact awkwardly with the[Params]magnitudes. Mirrors the earlierRapidFireOpenClosescaling change from Introduce new performance benchmark runners #4447..WithEnvironmentVariable("COMPlus_gcServer", "1")on the BDN job (which is a no-op underInProcessEmitToolchain, sinceCOMPlus_gcServeris a startup-only runtime knob) to<ServerGarbageCollection>true</ServerGarbageCollection>on the perf test.csprojso it actually applies to the host process. Addresses the Copilot review note.Checklist
Build
dotnet build src/Microsoft.Data.SqlClient/tests/PerformanceTests/Microsoft.Data.SqlClient.PerformanceTests.csproj -c Release→ 0 warnings, 0 errors onnet8.0/net9.0/net10.0.