Build the perf baseline from source instead of a pinned package - #4763
Closed
paulmedynski wants to merge 1 commit into
Closed
paulmedynski wants to merge 1 commit into
paulmedynski wants to merge 1 commit into
Conversation
The perf baseline pass compiled this commit's PerformanceTests and TestCommon sources against an older released driver, pinned via MdsPackageVersion. Test projects only support the driver at the same repo commit, so that is invalid: it only worked because the shared test code happened to touch public API only, and it blocks those projects from ever binding to driver internals. Resolve the requested baseline version to its v<version> release tag and build it through the existing source-baseline path, so the baseline ref's own test projects are built against its own driver source, exactly as the candidate pass builds this branch's. The baselineVersion parameter and the Kusto BASELINE_VERSION identity are unchanged, so baseline rows still record refs/tags/v<version>. Removes the now-unreachable package-baseline machinery: the released-package branch in both run scripts, the generated perf-baseline-nuget.config, MdsPackageVersion, and the NU1603 workaround for a specific preview baseline. The MDS_GE_<major> constants are now defined unconditionally rather than derived from the pinned version, because a benchmark still guards on MDS_GE_6 and dropping the constants would silently select its fallback path. Three consequences of comparing against a release tag are handled: - Baseline acquisition built a branch-only refspec, so a tag never resolved from the checkout's origin and every tagged run fell through to cloning the public fallback URL. Both helpers now try refs/heads and then refs/tags, mapping either into the private perfbaseline namespace. - Interleaved execution drives the benchmark app a unit at a time via PERF_LIST_BENCHMARKS/PERF_BENCHMARK, which arrived in v7.1.0-preview3. Every v7.0.x Program ignores both variables and runs the whole suite, so the default is raised to 7.1.0 and both scripts check the materialised baseline tree for the protocol, failing fast rather than emitting an invalid comparison. - A release tag can define a different benchmark registry, but interleave_perf.py enumerated units from the candidate alone and the baseline Program exits 2 for an unrecognised unit. It now enumerates both executables and pairs only the intersection, naming one-sided units in the log and recording them in comparison.json. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Tag-fetch diagnostics are incorrect, registry logic lacks committed tests, and documentation retains contradictory guidance.
Review effort: Balanced
Findings: 2
Open (4)
What changed in this PR
Moves performance baselines from released packages to release-tag source builds.
Changes:
- Builds baseline driver and tests from matching source tags.
- Adds tag fetching and harness-protocol validation.
- Compares only benchmark units shared by baseline and current builds.
| File | Description |
|---|---|
Microsoft.Data.SqlClient.PerformanceTests.csproj |
Removes package-baseline version pinning. |
sqlclient-perf-pipeline.yml |
Configures release-tag source baselines. |
sqlclient-perf-experiment.yml |
Updates baseline-selector documentation. |
run-perf-tests.sh |
Implements Linux source-baseline flow. |
run-perf-tests.ps1 |
Implements Windows source-baseline flow. |
interleave_perf.py |
Handles divergent benchmark registries. |
README.md |
Documents the new baseline model. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+517
to
+519
| $exit = Invoke-GitNetwork "fetch-tag" @( | ||
| "-C", $RepoRoot, "fetch", "--no-tags", "--depth", "1", "origin", | ||
| "+refs/tags/${Ref}:refs/remotes/perfbaseline/$Ref") |
Comment on lines
+568
to
+569
| || git_net fetch-tag -C "${REPO_ROOT}" fetch --no-tags --depth 1 origin \ | ||
| "+refs/tags/${ref}:refs/remotes/perfbaseline/${ref}"; then |
Comment on lines
+307
to
+309
| The cumulative `MDS_GE_<major>` constants remain defined unconditionally for benchmarks carried over | ||
| from the era of pinned-package baselines; they are effectively always true and the guards using them | ||
| can be removed: |
Comment on lines
+203
to
+207
| current = self._list_units_for(self.current_dir) | ||
| # The switch-under-test mode points both variants at one build, so skip the second | ||
| # enumeration: it would spawn another process only to return the same list. | ||
| if os.path.abspath(self.baseline_dir) == os.path.abspath(self.current_dir): | ||
| return current, [], [] |
Contributor
Author
|
Abandoning this approach. The perf baseline will remain a pinned released package; the underlying TestCommon/driver coupling is being addressed in #4759 instead. |
3 of 4 tasks
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
Moves the perf baseline from a pinned released package to the source of a release tag.
Stack: this is PR 1 of 2. #4759 (InternalsVisibleTo in
LocalAppContextSwitchesHelper) is stacked on top and targets this branch.Why
The baseline pass compiled this commit's
PerformanceTestsandTestCommonsources against an older released driver, pinned viaMdsPackageVersion. Test projects only support the driver at the same repo commit, so that is invalid. It worked only because the shared test code happened to touch public API only — and it blocks those projects from ever binding to driver internals, which is what #4759 needs.The repo already had the right mechanism: the PR perf pipeline builds a baseline ref's own perf project against its own driver source. This makes the main pipeline do the same, resolving
baselineVersionto itsv<version>release tag.baselineVersionand the KustoBASELINE_VERSIONidentity are unchanged, so baseline rows still recordrefs/tags/v<version>.What's removed
The now-unreachable package-baseline machinery: the released-package branch in both run scripts, the generated
perf-baseline-nuget.config,MdsPackageVersion, and an NU1603 workaround for a specific preview baseline.MDS_GE_<major>constants are now defined unconditionally rather than derived from the pin.JsonVsVarcharReadRunner.csstill guards onMDS_GE_6, so dropping them would have silently selected its fallback path.Three consequences of comparing against a tag
All three were found by Copilot review on #4759 and are fixed here:
Tags never resolved from origin. Both helpers fetched
--no-tagswith a branch-only refspec, so every tagged run fell through to cloning the public fallback URL even where the ADO origin was reachable. They now tryrefs/headsthenrefs/tags, mapping either into the privateperfbaselinenamespace.Old tags predate the harness protocol. Interleaved runs drive the app a unit at a time via
PERF_LIST_BENCHMARKS/PERF_BENCHMARK, which arrived inv7.1.0-preview3. Everyv7.0.xProgramignores both and runs the whole suite:v7.0.0–v7.0.3runnerconfig.jsonv7.1.0-preview3,v7.1.0runnerconfig.jsoncDefault raised
7.0.2→7.1.0, and both scripts check the materialised baseline tree for the protocol, failing fast rather than emitting an invalid comparison or running to timeout. Failing is preferred over auto-downgrading to sequential, which would change perf methodology invisibly.Benchmark registries can diverge.
interleave_perf.pyenumerated units from the candidate alone, and the baselineProgramexits 2 for an unrecognised unit — so a benchmark added since the tag failed the whole run. It now enumerates both executables, pairs only the intersection, names one-sided units in the log, and records them incomparison.jsonascurrentOnlyUnits/baselineOnlyUnits. Wholly disjoint registries remain an error, with a message saying why.--switch-under-testpoints both variants at one build, so it keeps its single enumeration.Validation
run-perf-tests.shbash -ncleanrun-perf-tests.ps1interleave_perf.pyPerformanceTests,TestCommon(Project + Package),UnitTestsnet8.0Harness-protocol guard verified against real tags: trips on
v7.0.2, passes onv7.1.0, trips on a missingProgram.cs.Reviewer notes
v7.1.0is unverified end-to-end. The behavioural change worth an owner's eye: the baseline tag must now be reachable fromoriginorbaselineRepoUrlon the VM, where previously only nuget.org access was needed.MDS_GE_*guards are now vestigial (always true) and could be removed along with their#elsebranches in a follow-up.