Batch DuckDB test seeding: shared connection + one transaction per seed helper - #1694
Merged
Conversation
…ed helper Profiling the analysis-heavy filter after the shared-fixture change showed the remaining cost was not schema DDL but seeding I/O: every seeded row opened a fresh DuckDB connection and auto-committed a single INSERT, measured at ~90ms/row, and AnomalyDetectorTests' 100-200-row seeds alone were the filter's entire critical path (154.5s serial of its ~154s wall). - AnomalyDetectorTests / BaselineProviderTests: a per-test-instance seed connection reused for every row; the seven SeedBaseline* loops in AnomalyDetectorTests each run inside one BEGIN/COMMIT. - TestDataSeeder: one lazily-opened connection per seeder instance, and every seed helper wraps its inserts in a SeedBatch (BEGIN on create, COMMIT on dispose) so a helper's rows share one WAL flush. ClearTestDataAsync deliberately stays un-batched: its per-table try/catch DELETEs would abort an explicit transaction on the first missing table and silently skip the rest of the clear. - TestDataSeeder is now IDisposable (it owns the connection); all 48 construction sites take `using var seeder`. Measured (local, idle box): analysis-heavy filter 153 tests 164s -> 19s; AnomalyDetectorTests 154.5s -> 5.9s serial; full suite 1494 tests 233s -> 78s. Full suite run twice consecutively with identical 1494/1494 results both times. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
pull Bot
pushed a commit
to ehtick/PerformanceMonitor
that referenced
this pull request
Jul 29, 2026
The split existed because the seven analysis classes rebuilt the full DuckDB schema inside every test and their subset alone cost ~9 CI minutes, so a narrower lite_analysis path gate let non-analysis Lite changes skip it. After the shared class fixtures (erikdarlingdata#1693, erikdarlingdata#1698) and batched seeding (erikdarlingdata#1694), that subset runs in ~66s on the same runner - the split no longer earns its second test-host spin-up, and the hand-maintained class-name filters were a drift risk (a renamed class would silently fall out of the heavy filter and into fast). One "Run Lite tests" step now runs the whole suite, gated on the lite path filter; the unconsumed lite_analysis filter block is removed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.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
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 follow-up #1693's profiling demanded: the analysis-heavy filter's remaining cost was not schema DDL but seeding I/O. Every seeded row opened a fresh DuckDB connection and auto-committed a single INSERT - measured at ~90ms per row - and
AnomalyDetectorTests' 100-200-row seeds alone were the filter's entire critical path (154.5s serial of its ~154s wall).Stacked on #1693 (auto-merge armed); once it lands, this diff collapses to the batching commits only.
What changed
AnomalyDetectorTests/BaselineProviderTests: a per-test-instance seed connection reused for every row; the sevenSeedBaseline*loops in AnomalyDetectorTests each run inside oneBEGIN/COMMIT.TestDataSeeder: one lazily-opened connection per seeder instance, and every seed helper wraps its inserts in aSeedBatch(BEGIN on create, COMMIT on dispose) so a helper's rows share one WAL flush instead of paying an auto-commit apiece.ClearTestDataAsyncdeliberately stays un-batched: its per-table try/catch DELETEs would abort an explicit transaction on the first missing table and silently skip the rest of the clear.TestDataSeederis nowIDisposable(it owns the connection); all 48 construction sites takeusing var seeder. Four classes outside the heavy filter (DatabaseFilter, FactScorer, FinOps, FindingStore) use the seeder too and get the same speedup for free.Measured (local, idle box, Debug, --no-build)
Per-class serial totals (trx): AnomalyDetector 154.5s -> 5.9s, FactCollector 116.8s -> 20.0s, Scenario 103.9s -> 17.2s, Misery 55.2s -> 10.2s, AnalysisService 32.1s -> 5.7s, Inference 25.9s -> 5.2s, BaselineProvider 30.0s -> 4.5s. No class above 20s serial.
For honesty's sake: #1693's own CI run showed the fixture alone was NET NEUTRAL on the runner ("Run Lite analysis-heavy tests" 612s vs the 556s reference, with the fast step drifting +10% the same way - runner variance). The per-row seeding cost was the real bottleneck all along; this PR is the one that should collapse that CI step.
Isolation proof
Same standard as #1693, re-proven here because transaction semantics changed:
SeedBatch.Disposecommits best-effort and swallows its own error so it can never mask the test's real exception.Generated with Claude Code