Fix perf test build against pre-6.0 MDS baselines - #4509
Open
cheenamalhotra wants to merge 4 commits into
Open
Conversation
JsonVsVarcharReadRunner used SqlDataReader.GetSqlJson, which was introduced in MDS 6.0. When the perf pipeline builds the test project in Package mode against an older pinned baseline (e.g. -p:MdsPackageVersion=5.2.3), compilation failed with CS1061. Define MDS_SQLJSON_SUPPORTED in the perf test project unless a pre-6.0 baseline package is pinned, and fall back to the string accessor in the JSON benchmarks when it is not defined. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 773aff6a-7a03-477e-8229-705eb9bc2f13
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 773aff6a-7a03-477e-8229-705eb9bc2f13
Follow-up to the GetSqlJson build break: * Replace the one-off MDS_SQLJSON_SUPPORTED constant with cumulative MDS_GE_<major> constants derived from MdsPackageVersion, so future benchmarks can guard any post-baseline API without new plumbing. Unparsable/absent versions fail toward "current" (all constants set). * Warn loudly from JsonVsVarcharReadRunner's GlobalSetup when the JSON fallback is compiled in, since the baseline pass then measures a different code path and its delta is not comparable. * Fail with an actionable message when the server has no native JSON column type instead of a raw SqlException. * Document the "benchmarks must compile against the oldest baseline" constraint and add a CS1061 troubleshooting row in the perf README. * Minor: make the row count a non-static instance field and drop trailing whitespace. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 773aff6a-7a03-477e-8229-705eb9bc2f13
Edge cases surfaced while reviewing the baseline API gating: * Tolerate leading whitespace and a "v" prefix in MdsPackageVersion when deriving the baseline major version. * Validate RowCount and the configured connection string in GlobalSetup instead of failing later with an opaque error. * Drop any tables created by a partially-failed GlobalSetup, since GlobalCleanup is not guaranteed to run when setup throws. * Attempt both drops in GlobalCleanup even if one fails, skip tables that were never created, still clear pools on failure, and aggregate the failures rather than leaking a table silently. * Only emit the pre-6.0 fallback warning for the JSON parameterization. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 773aff6a-7a03-477e-8229-705eb9bc2f13
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes the perf pipeline’s baseline build (Package mode with a pinned, released Microsoft.Data.SqlClient version) by ensuring the PerformanceTests project can compile against pre-6.0 baselines that don’t expose newer APIs like SqlDataReader.GetSqlJson. It does this via MSBuild-defined compile-time constants and runner-side API gating, while also improving benchmark setup/cleanup robustness to avoid leaking tables on failure.
Changes:
- Add MSBuild logic in
PerformanceTeststo derive a baseline major version fromMdsPackageVersionand define cumulativeMDS_GE_<major>constants (e.g.,MDS_GE_6,MDS_GE_7). - Gate
JsonVsVarcharReadRunner’s JSON accessor usage behind#if MDS_GE_6, with meaningful fallbacks and clearer/setup-time warnings when the fallback path is used. - Document the “oldest baseline must compile” rule and troubleshooting guidance in perf pipeline docs.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
src/Microsoft.Data.SqlClient/tests/PerformanceTests/Microsoft.Data.SqlClient.PerformanceTests.csproj |
Derives baseline major from MdsPackageVersion and defines MDS_GE_6 / MDS_GE_7 compile-time constants to enable API gating. |
src/Microsoft.Data.SqlClient/tests/PerformanceTests/BenchmarkRunners/JsonVsVarcharReadRunner.cs |
Guards GetSqlJson behind MDS_GE_6 with fallbacks; improves setup validation, failure cleanup, and drop robustness to reduce perf-server table leaks. |
eng/pipelines/perf/README.md |
Adds guidance for guarding post-baseline APIs with MDS_GE_<major> and links troubleshooting for baseline CS1061 build failures. |
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.
Summary
The perf pipeline's baseline pass compiles the
PerformanceTestsproject against a released MDS package (-p:ReferenceType=Package -p:MdsPackageVersion=<version>).JsonVsVarcharReadRunnercallsSqlDataReader.GetSqlJson, which was introduced in MDS 6.0, so any pre-6.0 baseline broke that pass:Reproduced locally with
-p:MdsPackageVersion=5.2.3; 6.0.2 / 7.0.2 / 7.1.0-preview build fine.Changes
Baseline API gating (
Microsoft.Data.SqlClient.PerformanceTests.csproj)MdsBaselineMajorVersionfromMdsPackageVersionand defines cumulativeMDS_GE_6/MDS_GE_7constants, so any future benchmark can guard a post-baseline API without new one-off plumbing.vprefix. Project mode, unpinned Package mode, and unparsable versions all fail toward "current" (sentinel99999), since those builds reference a current MDS.JsonVsVarcharReadRunner#if MDS_GE_6, falling back toGetString/GetFieldValueAsync<string>. The async fallback keeps an async read path so the async benchmark stays meaningful.GlobalSetupwhen the fallback is compiled in — the baseline then measures a different code path, so that benchmark's delta is not comparable.SqlException.RowCountand the configured connection string up front.GlobalSetup(BenchmarkDotNet does not guaranteeGlobalCleanupruns when setup throws, and table names are unique per run, so leaks accumulate on the perf server).GlobalCleanupattempts both drops even if one fails, skips tables never created, aggregates failures, and still clears pools viafinally._rowCount, trailing whitespace.Docs (
eng/pipelines/perf/README.md)CS1061in the baseline pass to the fix.Validation
Emitted constants:
MdsPackageVersion5.2.3,v5.2.3,V5.2.3,0.1.06.0.2MDS_GE_67.0.2,7.1.0-preview1.26124.5MDS_GE_6;MDS_GE_7latest(unparsable), unpinned, Project modeBuilds clean in Package mode for 5.2.3 / 6.0.2 / 7.0.2 / 7.1.0-preview1.26124.5 and in Project mode across net8.0/net9.0/net10.0.
No product code is touched, so no release note is needed.