Add focused changeset scanning benchmark - #354
Open
DanRod1999 wants to merge 30 commits into
Open
Conversation
The three-hour weekly suite cannot provide timely feedback while a transformer change is under review. Reconstruct a deterministic local fixture and measure only incremental synchronization so developers can collect comparable results in minutes. Refs #328 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Use an injected scenario factory so additional timed operations can share the same deterministic fixture and reporting lifecycle. Run the manual benchmark on Linux and record scenario identity in every report format. Refs #328 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Temporarily run the synthetic quick-performance workflow from the feature branch before workflow_dispatch is registered on the default branch. Refs #328 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Refs #328 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Groundwork for a second quick-performance scenario that measures ChangedInstanceIds.initialize over changeset files. The scan oracle predicts a scan result from the ops a recipe performed, replaying them through an independently written squash so the check does not validate the transformer against itself. It covers all six ChangedInstanceOps collections; verifying only elements would let aspect and relationship regressions pass silently. Scenario budget moves onto the scenario definition, replacing the three places the 15 minute figure was hardcoded. The workflow job timeout stays fixed: it covers checkout, install and build as well, so it is a different quantity from the measured budget. Reports now stamp the transformer and core-backend versions that ran the measurement, which a fixture descriptor cannot supply because it records the versions that generated the fixture instead. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Replace Mocha delayed registration with serialized Vitest collection and explicit worker lifecycle cleanup while preserving the weekly report and authentication contracts. Validated transformer build, performance test type-check, lint, and focused cleanup/registration tests. The credentialed suite has not been executed locally; live Bentley Hub runtime behavior remains to be validated by the weekly ADO pipeline. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
BenchmarkRunner rebuilt the entire fixture -- seeds, hub, iModels, changesets -- inside the sample loop, so a run paid that cost 9 times. On balanced-incremental that is 94s of reconstruction to measure 1.4s of work, and a heavier changeset-scanning fixture would not fit the 15-minute budget at all. It also made A/B comparison impossible: two arms could not measure the same fixture. Split fixture handling into two stages behind a FixtureProvider seam. Stage 1 builds an immutable artifact once; stage 2 hands each sample a fresh filesystem copy. Samples still start pristine -- only the expensive generation moved out of the loop, not the isolation. Which stage does the real work depends on topology, not recipe: - source-and-empty-target keeps the live hub. Its measured region re-enters BriefcaseManager and pushes provenance to a target briefcase, so it cannot be captured as relocatable bytes. Stage 1 is a no-op there and behaviour is unchanged. - source-only captures the briefcase and its changeset files. This works because BriefcaseDb.open makes no hub calls and ChangedInstanceIds accepts csFileProps without one -- capturing csFileProps is the only hub-free input mode, not an optimization. Measured: 5.3s build once, 29ms per sample thereafter. Fixture selection was hardcoded at both entry points and getFixtureDescriptor threw on anything but balanced-incremental. Scenarios now declare a default fixture id and capabilities; resolution validates the resolved pair. Capabilities validate, they do not select. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Adds the recipe body and schema for the changeset-scanning scenario, plus ledger serialization so the oracle's expectation can cross the fixture build/measure boundary. Elements are partitioned into four regions so every id has one unambiguous expected outcome after ChangedInstanceIds squashes the scanned range, including a region that is inserted and then deleted and must therefore cancel out entirely. Verified against a live HubMock at five shapes: the oracle matches the scan exactly across all six ChangedInstanceOps collections. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Stage 2 is only a filesystem copy because a briefcase's bytes are portable and BriefcaseDb.open consults no hub. That property was asserted incidentally inside a broader test, so a regression would have surfaced as a confusing benchmark failure rather than a clear one. Assert it directly: relocate the briefcase to a new path, open it readonly with HubMock shut down, and confirm it still advertises the briefcase id and parent changeset of an iModel that no longer exists anywhere. openDgnDb makes no hub calls today, but nothing in core-backend's contract promises it never will. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
A scanning oracle needs the exact id sets its recipe operated on, and there
was no path for recipe-produced data to reach a scenario. The ids cannot be
recovered later: the artifact briefcase is pinned at tip, so every element
the recipe deleted is already gone from it, and deriving them from the
changeset files would be circular for a scenario whose job is to verify
those same files.
applySourceChangesets may now return a value. Stage 1 serializes it to
recipe.json in the artifact and records the file in the manifest; stage 2
surfaces it as PreparedDetachedDataset.recipe, typed unknown so the
framework never interprets it. Recipes that return nothing are untouched
and the key stays absent, so the artifact version stays at 1.
Validation walks the returned value and rejects anything JSON cannot carry.
A round-trip comparison would not do: JSON.stringify(new Set()) is "{}", and
both sides then have zero enumerable keys, so a lossy Set would compare
equal and an oracle would later read an empty id list as a pass.
Because the artifact is built once and copied, recipe.json is byte-identical
across both arms of an A/B run, so an expectation cannot drift between arms
and turn a correctness disagreement into an unexplained verdict.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Adds a second quick-performance scenario measuring
ChangedInstanceIds.initialize({iModel, csFileProps}) over local changeset
files, with no target iModel and no hub on the measured path.
The measured region is deliberately narrow. IModelTransformer.initialize
also does provenance init, clone-context and exporter setup and deleted
entity remapping; scanning is roughly a third of it, so at ~3% run-to-run
variation a 20% scan regression would move that larger total by ~6% and
vanish into the noise. Measured alone it sits at 2.4% CV over 8 samples.
Two hooks were added to FixtureRecipe because a second recipe made two
implicit assumptions visible:
- applySourceChangesets may now return JSON-serializable recipe data,
captured as recipe.json in the artifact and handed back to the scenario.
A recipe runs while the artifact is built and a scenario runs later
against a copy, so the scan oracle ledger had no way across that
boundary. Deleted ids in particular cannot be recovered by querying the
tip briefcase, and three of the six collections are delete sets.
- validate() is now recipe-owned. assertFixtureDistribution queries
QuickPerf.Balanced* classes and updated-% labels, so it was silently
specific to one recipe while being called for all of them.
Verification is a recipe oracle rather than a digest: the recipe records
every operation it performs and the scenario asserts exact set equality
across all six ChangedInstanceOps collections. A digest can only detect
that behaviour changed, not that it was wrong from the start. The digest
is still returned, as a cross-arm behaviour gate.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
The ledger deduplicated repeated ops on the stated grounds that all three squash rules are idempotent under repetition. That is false for Deleted. Its branch in ChangedInstanceIds.handleChange is conditional on insertIds.has(id), and the first delete falsifies that condition: Inserted -> Deleted => id in no set (the two cancel) Inserted -> Deleted -> Deleted => id in deleteIds So deduplicating a repeated delete silently produces the wrong expectation. The current recipe never emits one, but the justification was unsound, which would have mislead the next recipe author. Repeated deletes are not producible from a valid changeset sequence, so the ledger now rejects a second Deleted for an id as a recipe authoring error rather than deduplicating it. Rejecting is strictly more informative than deduping here. Inserts and updates are genuinely idempotent and are still deduplicated; the scale argument for that is unaffected. squashLedger now also accepts raw entries so that squash semantics can be pinned directly by tests, including sequences record() refuses. Adds tests pinning the non-idempotent case and both double-membership sequences the transformer produces. See the accompanying finding on handleChange's incomplete reconciliation: the oracle mirrors that behaviour deliberately rather than normalizing it. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
W1 and W3 implemented the recipe-data hook independently. W1's version is
kept wholesale and W3's removed, because it is better on both refinements
that were requested:
Presence is recorded as a manifest recipeDataFile key rather than probed
for on disk, and readFixtureArtifact fails when the manifest declares data
the artifact does not contain.
Validation walks the returned value and requires JSON-native types at every
node, rather than comparing a JSON round-trip. The distinction matters:
JSON.stringify(new Set()) is "{}", so both sides of a round-trip comparison
would have zero enumerable keys and a lossy Set would compare equal. An
oracle would then read an empty id list as a pass.
Kept from W3: the recipe-owned validate hook, which W1's interface predates.
Its stub recipe gains a validate delegating to balancedIncremental.
The scan recipe's payload is now typed through W1's new TArtifactData
parameter instead of unknown. It returns a plain array of plain
string-valued objects, so it passes the JSON-native walk; returning the
ScanLedger itself would correctly fail, as it holds a private Set.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Verified against a real 8-sample run (varianceStatus stable, CV 1.0%): build 19.3s, median scan 3.46s, ~51ms per-sample overhead. Updates the doc comment's numbers, which were still describing scanScale 11, and corrects the 128-execution headroom claim from ~3x to ~2x against the 15-minute budget at this shape.
…elHost profiles
Adds a regression test proving a recipe returning a Set (which
JSON.stringify silently collapses to "{}") fails the build loudly
instead of emitting a half-written artifact that reads back as valid.
Wires in startIsolatedHost/shutdownIsolatedHost so these correctness
tests run on a private IModelHost profile instead of the default one,
which is exclusively locked and shared by every iTwin.js process on
the machine -- concurrent runs otherwise fail with an unrelated-looking
"Db is busy" error.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
This was referenced Jul 28, 2026
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Keep credential loading in the Hub-backed regression suite so local helper unit tests run without CI or a .env file. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Use IModelHost validity for partial-startup cleanup and keep unlimited timeouts local to the credentialed weekly regression file. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Rely on Vitest's per-test reporting instead of emitting an unstructured completion message. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Explain the weekly regression lifecycle, helper unit tests, registration matrix, cleanup, reporting contract, and extension boundaries. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
DanRod1999
added a commit
that referenced
this pull request
Jul 29, 2026
## Summary - migrate the weekly performance regression suite from Mocha to serialized Vitest execution - remove performance-tests ownership of Mocha, `@types/mocha`, Chai 4, and `ts-node` - preserve the weekly report and authentication contracts ### New test helpers These helpers are not required by Vitest. They make behavior that was embedded in the previous Mocha suite explicit and independently testable: - `Cleanup.ts` guarantees that every registered teardown task is attempted, including database closure, report export, `IModelHost` shutdown, and authorization sign-out. If the operation and cleanup both fail, it preserves both errors instead of allowing teardown to hide the primary failure. - `RegressionTestRegistration.ts` constructs the supported test-case/transformer-module combinations synchronously before Vitest registers tests. It skips modules that do not implement the operation required by a test case, preserving the old conditional Mocha registration behavior without asynchronous collection callbacks. - `Cleanup.test.ts` and `RegressionTestRegistration.test.ts` provide focused, non-credentialed coverage for those behaviors. This PR targets `main` and is intended as the new bottom layer beneath the existing quick-performance PRs #353, #354, #356, and #357. It does not modify those branches or their stack metadata. ## Validation - `pnpm --dir packages/transformer build` - `pnpm --dir packages/performance-tests build` - `pnpm --dir packages/performance-tests lint` - `CI=true pnpm --dir packages/performance-tests exec vitest run test/unit/Cleanup.test.ts test/unit/RegressionTestRegistration.test.ts` (4 tests passed) - verified the performance-tests manifest and lockfile importer no longer declare Mocha, `@types/mocha`, Chai 4, or `ts-node` The credentialed Hub-backed weekly runtime suite was intentionally not run locally. Hosted weekly Azure validation remains deferred, consistent with the source migration handoff. *generated with GPT-5.6 Sol* --------- Co-authored-by: Nam Le <50554904+hl662@users.noreply.github.com> Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Co-authored-by: Daniel Rodriguez <DanRod1999@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
hl662
reviewed
Jul 29, 2026
| * shape is a single `scanScale` knob. | ||
| * | ||
| * The size is bounded by comparison mode rather than by a single run. One run is 9 executions and | ||
| * finishes in well under a minute (measured: 52 s including the 19.3 s build), but an A/B |
Contributor
There was a problem hiding this comment.
🤖 Nambot (powered by gpt-5.6-sol) — The calibration rationale no longer matches the measured or implemented operating model. This PR reports a 32.50-second median rather than 3.46 seconds, and the current comparison workflow runs eight sequential executions per pair job rather than an escalated block of 128. Can we replace these figures with current measurements and explain the actual per-job comparison width? The fixture still fits the current budget; the concern is that this comment gives maintainers the wrong basis for changing scanScale.
Nambot 🤖 (powered by gpt-5.6-sol)
Contributor
There was a problem hiding this comment.
don't put concrete numbers in the fixture catalog documentation (this might be my bot's mistake oops)
Just napkin math estimates is fine
| // Regions C and D, both inserted in the first changeset. | ||
| inserts: 30 * scanScale, | ||
| // Every seeded element is updated, plus region C after its insert. | ||
| updates: 240 * scanScale, |
Contributor
There was a problem hiding this comment.
🤖 Nambot (powered by gpt-5.6-sol) — These values count distinct IDs touched, not update operations performed. At scale 16 over 20 changesets, the recipe issues about 76,160 element and 70,080 aspect update calls, while emitted samples label 3,840 and 3,520 as operations. Can we either report the actual scheduled operations, or rename and document these as distinct affected IDs and add separate raw operation/change-row counts?
Nambot 🤖 (powered by gpt-5.6-sol)
| * | ||
| * The measured region is deliberately narrow. `IModelTransformer.initialize` also performs | ||
| * provenance initialization, clone-context and exporter setup and deleted-entity remapping, so a | ||
| * regression in any of those would surface here as a scanning regression, and — more importantly — |
Contributor
There was a problem hiding this comment.
🤖 Nambot (powered by gpt-5.6-sol) — This scenario times only ChangedInstanceIds.initialize(), so regressions in provenance initialization, clone-context/exporter setup, or deleted-entity remapping cannot surface here. Can we narrow this explanation to say that the focused region isolates scanning regressions from those other initialization costs?
Nambot 🤖 (powered by gpt-5.6-sol)
| * would move the total by about 6% and disappear into the noise. Narrowing the region is what makes | ||
| * the regression detectable. | ||
| * | ||
| * `csFileProps` is the only one of the four `initialize` input modes that touches no hub: the other |
Contributor
There was a problem hiding this comment.
🤖 Nambot (powered by gpt-5.6-sol) — csFileProps is not the only hub-free input mode: supplying precomputed changedInstanceIds returns without querying BriefcaseManager. Could this say that csFileProps is the hub-free mode that still exercises changeset-file scanning?
Nambot 🤖 (powered by gpt-5.6-sol)
| ); | ||
| }); | ||
|
|
||
| it("covers all six ChangedInstanceIds collections", () => { |
Contributor
There was a problem hiding this comment.
🤖 Nambot (powered by gpt-5.6-sol) — This test proves that the collection-name list contains six entries, but the generated fixture has no positive font change. Could we narrow the test name to the registry shape, or add positive font coverage if the benchmark is intended to protect all six scanner collections?
Nambot 🤖 (powered by gpt-5.6-sol)
| private readonly _entries: ScanLedgerEntry[] = []; | ||
| private readonly _lastOp = new Map<string, ScanOp>(); | ||
| private readonly _deleted = new Set<string>(); | ||
|
|
Contributor
There was a problem hiding this comment.
succinct doccomments, use plannotator's write-better skill for any exposed public method.
| export interface BenchmarkScenarioDefinition { | ||
| readonly id: string; | ||
| readonly defaultFixtureId: string; | ||
| readonly capabilities: BenchmarkScenarioCapabilities; | ||
| readonly factory: BenchmarkScenarioFactory; | ||
| /** Wall-clock budget for the whole run, in milliseconds. */ | ||
| /** |
Contributor
There was a problem hiding this comment.
doccomments for every attribute in here would be nice, especially given we have 2 id attributes
| "utf8" | ||
| ), | ||
| lockfile: fs.readFileSync( | ||
| path.resolve( |
| expect([...withSecondDelete.element.deleteIds]).to.deep.equal(["0x42"]); | ||
| }); | ||
|
|
||
| // The transformer's Inserted branch clears deleteIds but not updateIds, and its Updated branch |
Contributor
There was a problem hiding this comment.
i feel like we need to separate cleanlier what tests make up the actual performance tests, and which ones are to assert and validate the fixtures and test setups are correct, and then we can divvy them up. I could see our typical CI unit test workflow running the unit tests for our quick pipelines stuff, but not run the perf tests (and instead run it in a spearate workflow)
correctness vs performance
| * See LICENSE.md in the project root for license terms and full copyright notice. | ||
| *--------------------------------------------------------------------------------------------*/ | ||
|
|
||
| import * as fs from "fs"; |
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Keep benchmark validation focused on exercising changeset scanning while leaving transformer semantics to transformer tests. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
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
2 participants
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
ChangedInstanceIds.initializebenchmark over local changeset filesDetailed
ChangedInstanceIdsinsert/update/delete semantics remain covered by the transformer package tests; this PR does not add a separate performance-layer behavior oracle or ledger.Validation
changeset-scanningrun: one measured sample plus warm-up passedgenerated with GPT-5.6 Sol