ci: benchmark PRs against main with the incremental update suite#1650
ci: benchmark PRs against main with the incremental update suite#1650kevin-dp wants to merge 3 commits into
Conversation
Extracts the incremental update benchmark from #1648 (with the opt-in perf tracing removed, since those hooks only exist on that branch) and adds a workflow that runs it on non-draft PRs touching packages/db, packages/db-ivm, or the benchmark itself. The workflow benchmarks the PR merge commit and its merge-base with the same benchmark code, then posts a comparison as a job summary and sticky PR comment. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Per-row significance flags miss consistent sub-floor shifts across many cases; the geometric mean of per-case median ratios surfaces them. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Incremental update benchmarkComparing 1 regression(s), 0 improvement(s) (threshold: ±20% and >0.05ms). Overall median write time vs base: 1.02× (geometric mean of per-case ratios; lower is faster).
Each row aggregates the 12 scale/index/write-mode configurations of that query; per-configuration tables below. 100 rows/collection | source indexes: none | synced writes — geomean 1.05×
100 rows/collection | source indexes: none | optimistic writes — geomean 0.98×
100 rows/collection | source indexes: manual | synced writes — geomean 1.48×, 1 change(s)
100 rows/collection | source indexes: manual | optimistic writes — geomean 1.06×
1,000 rows/collection | source indexes: none | synced writes — geomean 1.08×
1,000 rows/collection | source indexes: none | optimistic writes — geomean 0.87×
1,000 rows/collection | source indexes: manual | synced writes — geomean 0.89×
1,000 rows/collection | source indexes: manual | optimistic writes — geomean 1.00×
10,000 rows/collection | source indexes: none | synced writes — geomean 0.99×
10,000 rows/collection | source indexes: none | optimistic writes — geomean 0.92×
10,000 rows/collection | source indexes: manual | synced writes — geomean 1.08×
10,000 rows/collection | source indexes: manual | optimistic writes — geomean 0.98×
Runner: node v24.8.0, linux 6.17.0-1018-azure, AMD EPYC 9V74 80-Core Processor. Timings on shared CI runners are noisy; treat small deltas as indicative only. |
More templates
@tanstack/angular-db
@tanstack/browser-db-sqlite-persistence
@tanstack/capacitor-db-sqlite-persistence
@tanstack/cloudflare-durable-objects-db-sqlite-persistence
@tanstack/db
@tanstack/db-ivm
@tanstack/db-sqlite-persistence-core
@tanstack/electric-db-collection
@tanstack/electron-db-sqlite-persistence
@tanstack/expo-db-sqlite-persistence
@tanstack/node-db-sqlite-persistence
@tanstack/offline-transactions
@tanstack/powersync-db-collection
@tanstack/query-db-collection
@tanstack/react-db
@tanstack/react-native-db-sqlite-persistence
@tanstack/rxdb-db-collection
@tanstack/solid-db
@tanstack/svelte-db
@tanstack/tauri-db-sqlite-persistence
@tanstack/trailbase-db-collection
@tanstack/vue-db
commit: |
|
Size Change: 0 B Total Size: 125 kB ℹ️ View Unchanged
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a benchmark runner for incremental-update scenarios, a comparison script for base versus candidate reports, and a GitHub Actions workflow that runs both on pull requests and manual dispatches, publishes artifacts, and updates a PR comment. ChangesBenchmark Suite
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant GitHub as GitHub Actions
participant Benchmark as benchmark.yml
participant Runner as incremental-update.ts
participant Compare as compare-incremental-update.ts
participant PullRequest as PR comment
GitHub->>Benchmark: trigger on PR or dispatch
Benchmark->>Runner: generate candidate.json and base.json
Benchmark->>Compare: generate comparison.md
Benchmark->>PullRequest: post or update comment
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Size Change: 0 B Total Size: 4.26 kB ℹ️ View Unchanged
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
scripts/bench/compare-incremental-update.ts (2)
97-108: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueKey joining without escaping risks collisions.
resultKeyconcatenates fields with|without any escaping or type prefixes. If any label/query string ever contains|, boundaries between fields shift and unrelated results could be matched incorrectly.As per coding guidelines, "When converting keys to strings, ensure no collisions by using proper encoding with type prefixes."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/bench/compare-incremental-update.ts` around lines 97 - 108, The resultKey helper is building a composite string key by joining raw RunResult fields with a plain delimiter, which can collide if any value contains that delimiter. Update resultKey to use a collision-safe encoding with explicit type prefixes or escaped/serialized fields so each RunResult property remains unambiguous, and keep the matching logic in compare-incremental-update.ts aligned with the new key format.Source: Coding guidelines
159-179: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
isRegression/isImprovementare near-duplicate logic.Both functions share the same significance check and only differ in ratio direction; could be collapsed into a single
classifyhelper returning'regression' | 'improvement' | 'none'to avoid recomputingisSignificanttwice per comparison informatMarkdown.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/bench/compare-incremental-update.ts` around lines 159 - 179, The isRegression and isImprovement helpers duplicate the same significance logic and cause isSignificant to be evaluated twice for each Comparison in formatMarkdown. Refactor the comparison classification in compare-incremental-update.ts into a single helper (for example, a classify-style function) that checks significance once and returns regression, improvement, or none based on medianRatio direction, then update formatMarkdown to use that shared result instead of calling both helpers separately..github/workflows/benchmark.yml (1)
22-28: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueConsider a job-level timeout.
No
timeout-minutesis set; a hungpnpm install/build/benchmark could occupy the runner up to GitHub's default (6h) before failing.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/benchmark.yml around lines 22 - 28, The benchmark job currently has no execution limit, so a hung install/build/benchmark can hold a runner until GitHub’s default timeout. Add a job-level timeout setting to the benchmark job in the workflow definition alongside the existing if/runs-on fields, using the benchmark job identifier to keep the limit explicit and prevent runaway runs.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/bench/compare-incremental-update.ts`:
- Around line 15-52: The local type copies in compare-incremental-update.ts are
drifting from the shared incremental-update.ts contract, and
sourceIndexMode/mutationMode have been widened to string. Replace the duplicated
Summary, FixtureScale, RunResult, and Report definitions by importing the shared
types from incremental-update.ts, or export them there if needed, so
compare-incremental-update stays aligned with the canonical SourceIndexMode and
MutationMode unions and preserves type safety.
- Around line 204-296: The footer in formatMarkdown is incorrectly reading
runtime data from the outer candidate closure instead of the candidateReport
parameter, which breaks the function’s explicit data flow. Update the Runner
line to use candidateReport.metadata for node, platform, and cpu so
formatMarkdown depends only on its arguments and remains testable/reusable. Keep
the rest of the function consistent with the existing use of baseReport and
candidateReport.
---
Nitpick comments:
In @.github/workflows/benchmark.yml:
- Around line 22-28: The benchmark job currently has no execution limit, so a
hung install/build/benchmark can hold a runner until GitHub’s default timeout.
Add a job-level timeout setting to the benchmark job in the workflow definition
alongside the existing if/runs-on fields, using the benchmark job identifier to
keep the limit explicit and prevent runaway runs.
In `@scripts/bench/compare-incremental-update.ts`:
- Around line 97-108: The resultKey helper is building a composite string key by
joining raw RunResult fields with a plain delimiter, which can collide if any
value contains that delimiter. Update resultKey to use a collision-safe encoding
with explicit type prefixes or escaped/serialized fields so each RunResult
property remains unambiguous, and keep the matching logic in
compare-incremental-update.ts aligned with the new key format.
- Around line 159-179: The isRegression and isImprovement helpers duplicate the
same significance logic and cause isSignificant to be evaluated twice for each
Comparison in formatMarkdown. Refactor the comparison classification in
compare-incremental-update.ts into a single helper (for example, a
classify-style function) that checks significance once and returns regression,
improvement, or none based on medianRatio direction, then update formatMarkdown
to use that shared result instead of calling both helpers separately.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: db0f6f77-810d-4a17-a5e2-33d8c19ebc76
📒 Files selected for processing (3)
.github/workflows/benchmark.ymlscripts/bench/compare-incremental-update.tsscripts/bench/incremental-update.ts
| type Summary = { | ||
| iterations: number | ||
| medianMs: number | ||
| p75Ms: number | ||
| p95Ms: number | ||
| minMs: number | ||
| maxMs: number | ||
| stddevMs: number | ||
| } | ||
|
|
||
| type FixtureScale = { | ||
| label: string | ||
| issueCount: number | ||
| userCount: number | ||
| commentCount: number | ||
| } | ||
|
|
||
| type RunResult = { | ||
| query: string | ||
| scenario: string | ||
| scale: FixtureScale | ||
| sourceIndexMode: string | ||
| mutationMode: string | ||
| coldHydrateMs: number | ||
| writeSummary: Summary | ||
| } | ||
|
|
||
| type Report = { | ||
| metadata: { | ||
| node: string | ||
| platform: string | ||
| cpu: string | ||
| gitSha: string | ||
| iterations: number | ||
| warmup: number | ||
| } | ||
| results: Array<RunResult> | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Duplicated types drift from the upstream contract.
Summary, FixtureScale, and RunResult are copy-pasted from scripts/bench/incremental-update.ts. Worse, sourceIndexMode/mutationMode are widened here to string instead of the SourceIndexMode/MutationMode literal unions used upstream (per the shared contract in incremental-update.ts:85-121), so this file loses type safety and both copies can silently diverge as the runner evolves.
Consider exporting the shared types from incremental-update.ts and importing them here.
As per coding guidelines, "Extract common logic into utility functions when identical or near-identical code blocks appear in multiple places."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/bench/compare-incremental-update.ts` around lines 15 - 52, The local
type copies in compare-incremental-update.ts are drifting from the shared
incremental-update.ts contract, and sourceIndexMode/mutationMode have been
widened to string. Replace the duplicated Summary, FixtureScale, RunResult, and
Report definitions by importing the shared types from incremental-update.ts, or
export them there if needed, so compare-incremental-update stays aligned with
the canonical SourceIndexMode and MutationMode unions and preserves type safety.
Source: Coding guidelines
| function formatMarkdown( | ||
| baseReport: Report, | ||
| candidateReport: Report, | ||
| allComparisons: Array<Comparison>, | ||
| threshold: number, | ||
| ): string { | ||
| const lines: Array<string> = [] | ||
| const regressed = allComparisons.filter((comparison) => | ||
| isRegression(comparison, threshold), | ||
| ) | ||
| const improved = allComparisons.filter((comparison) => | ||
| isImprovement(comparison, threshold), | ||
| ) | ||
|
|
||
| lines.push(`## Incremental update benchmark`) | ||
| lines.push(``) | ||
| lines.push( | ||
| `Comparing \`${candidateReport.metadata.gitSha}\` (this PR) against \`${baseReport.metadata.gitSha}\` (base). ` + | ||
| `Times are per-write medians over ${candidateReport.metadata.iterations} iterations ` + | ||
| `(${candidateReport.metadata.warmup} warmup writes).`, | ||
| ) | ||
| lines.push(``) | ||
|
|
||
| if (regressed.length === 0 && improved.length === 0) { | ||
| lines.push( | ||
| `**No significant changes** (threshold: ±${Math.round(threshold * 100)}% and >${absoluteFloorMs}ms).`, | ||
| ) | ||
| } else { | ||
| lines.push( | ||
| `**${regressed.length} regression(s), ${improved.length} improvement(s)** ` + | ||
| `(threshold: ±${Math.round(threshold * 100)}% and >${absoluteFloorMs}ms).`, | ||
| ) | ||
| } | ||
| lines.push(``) | ||
| lines.push( | ||
| `Overall median write time vs base: **${formatRatio( | ||
| geomeanRatio(allComparisons), | ||
| )}** (geometric mean of per-case ratios; lower is faster).`, | ||
| ) | ||
| lines.push(``) | ||
|
|
||
| const groups = new Map<string, Array<Comparison>>() | ||
| for (const comparison of allComparisons) { | ||
| const groupKey = `${formatScale(comparison.scale)} | source indexes: ${ | ||
| comparison.sourceIndexMode | ||
| } | ${comparison.mutationMode} writes` | ||
| const group = groups.get(groupKey) ?? [] | ||
| group.push(comparison) | ||
| groups.set(groupKey, group) | ||
| } | ||
|
|
||
| for (const [groupKey, group] of groups) { | ||
| lines.push(`<details>`) | ||
| const changed = group.filter( | ||
| (comparison) => marker(comparison, threshold) !== ``, | ||
| ).length | ||
| const changedSuffix = changed > 0 ? `, ${changed} change(s)` : `` | ||
| lines.push( | ||
| `<summary><b>${groupKey}</b> — geomean ${formatRatio( | ||
| geomeanRatio(group), | ||
| )}${changedSuffix}</summary>`, | ||
| ) | ||
| lines.push(``) | ||
| lines.push( | ||
| `| Query | Base median | PR median | Δ median | Base p95 | PR p95 | Δ p95 | |`, | ||
| ) | ||
| lines.push(`| --- | ---: | ---: | ---: | ---: | ---: | ---: | --- |`) | ||
| for (const comparison of group) { | ||
| lines.push( | ||
| `| ${comparison.query} | ${formatMs( | ||
| comparison.base.writeSummary.medianMs, | ||
| )} | ${formatMs(comparison.candidate.writeSummary.medianMs)} | ${formatDelta( | ||
| comparison.medianRatio, | ||
| )} | ${formatMs(comparison.base.writeSummary.p95Ms)} | ${formatMs( | ||
| comparison.candidate.writeSummary.p95Ms, | ||
| )} | ${formatDelta(comparison.p95Ratio)} | ${marker( | ||
| comparison, | ||
| threshold, | ||
| )} |`, | ||
| ) | ||
| } | ||
| lines.push(``) | ||
| lines.push(`</details>`) | ||
| lines.push(``) | ||
| } | ||
|
|
||
| lines.push( | ||
| `<sub>Runner: node ${candidate.metadata.node}, ${candidate.metadata.platform}, ${candidate.metadata.cpu}. ` + | ||
| `Timings on shared CI runners are noisy; treat small deltas as indicative only.</sub>`, | ||
| ) | ||
|
|
||
| return lines.join(`\n`) | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Uses closure variable candidate instead of the candidateReport parameter.
Lines 290-293 reference the top-level candidate variable rather than the candidateReport parameter that the rest of the function correctly uses (e.g. lines 221-224). It happens to work today because they point to the same object, but it breaks the function's parameter contract and hurts testability if this function is ever reused/tested independently.
As per coding guidelines, "Prefer explicit function parameters over closures to improve testability and clarify data flow dependencies."
♻️ Proposed fix
lines.push(
- `<sub>Runner: node ${candidate.metadata.node}, ${candidate.metadata.platform}, ${candidate.metadata.cpu}. ` +
+ `<sub>Runner: node ${candidateReport.metadata.node}, ${candidateReport.metadata.platform}, ${candidateReport.metadata.cpu}. ` +
`Timings on shared CI runners are noisy; treat small deltas as indicative only.</sub>`,
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function formatMarkdown( | |
| baseReport: Report, | |
| candidateReport: Report, | |
| allComparisons: Array<Comparison>, | |
| threshold: number, | |
| ): string { | |
| const lines: Array<string> = [] | |
| const regressed = allComparisons.filter((comparison) => | |
| isRegression(comparison, threshold), | |
| ) | |
| const improved = allComparisons.filter((comparison) => | |
| isImprovement(comparison, threshold), | |
| ) | |
| lines.push(`## Incremental update benchmark`) | |
| lines.push(``) | |
| lines.push( | |
| `Comparing \`${candidateReport.metadata.gitSha}\` (this PR) against \`${baseReport.metadata.gitSha}\` (base). ` + | |
| `Times are per-write medians over ${candidateReport.metadata.iterations} iterations ` + | |
| `(${candidateReport.metadata.warmup} warmup writes).`, | |
| ) | |
| lines.push(``) | |
| if (regressed.length === 0 && improved.length === 0) { | |
| lines.push( | |
| `**No significant changes** (threshold: ±${Math.round(threshold * 100)}% and >${absoluteFloorMs}ms).`, | |
| ) | |
| } else { | |
| lines.push( | |
| `**${regressed.length} regression(s), ${improved.length} improvement(s)** ` + | |
| `(threshold: ±${Math.round(threshold * 100)}% and >${absoluteFloorMs}ms).`, | |
| ) | |
| } | |
| lines.push(``) | |
| lines.push( | |
| `Overall median write time vs base: **${formatRatio( | |
| geomeanRatio(allComparisons), | |
| )}** (geometric mean of per-case ratios; lower is faster).`, | |
| ) | |
| lines.push(``) | |
| const groups = new Map<string, Array<Comparison>>() | |
| for (const comparison of allComparisons) { | |
| const groupKey = `${formatScale(comparison.scale)} | source indexes: ${ | |
| comparison.sourceIndexMode | |
| } | ${comparison.mutationMode} writes` | |
| const group = groups.get(groupKey) ?? [] | |
| group.push(comparison) | |
| groups.set(groupKey, group) | |
| } | |
| for (const [groupKey, group] of groups) { | |
| lines.push(`<details>`) | |
| const changed = group.filter( | |
| (comparison) => marker(comparison, threshold) !== ``, | |
| ).length | |
| const changedSuffix = changed > 0 ? `, ${changed} change(s)` : `` | |
| lines.push( | |
| `<summary><b>${groupKey}</b> — geomean ${formatRatio( | |
| geomeanRatio(group), | |
| )}${changedSuffix}</summary>`, | |
| ) | |
| lines.push(``) | |
| lines.push( | |
| `| Query | Base median | PR median | Δ median | Base p95 | PR p95 | Δ p95 | |`, | |
| ) | |
| lines.push(`| --- | ---: | ---: | ---: | ---: | ---: | ---: | --- |`) | |
| for (const comparison of group) { | |
| lines.push( | |
| `| ${comparison.query} | ${formatMs( | |
| comparison.base.writeSummary.medianMs, | |
| )} | ${formatMs(comparison.candidate.writeSummary.medianMs)} | ${formatDelta( | |
| comparison.medianRatio, | |
| )} | ${formatMs(comparison.base.writeSummary.p95Ms)} | ${formatMs( | |
| comparison.candidate.writeSummary.p95Ms, | |
| )} | ${formatDelta(comparison.p95Ratio)} | ${marker( | |
| comparison, | |
| threshold, | |
| )} |`, | |
| ) | |
| } | |
| lines.push(``) | |
| lines.push(`</details>`) | |
| lines.push(``) | |
| } | |
| lines.push( | |
| `<sub>Runner: node ${candidate.metadata.node}, ${candidate.metadata.platform}, ${candidate.metadata.cpu}. ` + | |
| `Timings on shared CI runners are noisy; treat small deltas as indicative only.</sub>`, | |
| ) | |
| return lines.join(`\n`) | |
| } | |
| lines.push( | |
| `<sub>Runner: node ${candidateReport.metadata.node}, ${candidateReport.metadata.platform}, ${candidateReport.metadata.cpu}. ` + | |
| `Timings on shared CI runners are noisy; treat small deltas as indicative only.</sub>`, | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/bench/compare-incremental-update.ts` around lines 204 - 296, The
footer in formatMarkdown is incorrectly reading runtime data from the outer
candidate closure instead of the candidateReport parameter, which breaks the
function’s explicit data flow. Update the Runner line to use
candidateReport.metadata for node, platform, and cpu so formatMarkdown depends
only on its arguments and remains testable/reusable. Keep the rest of the
function consistent with the existing use of baseReport and candidateReport.
Source: Coding guidelines
The per-configuration tables are collapsed, so the comment showed no per-query numbers without expanding; a visible rollup (geomean, spread, flags per query) carries the signal. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Summary
Extracts the incremental update benchmark from #1648 into a standalone suite and adds a CI workflow that automatically measures the performance impact of PRs against
main.What's included
scripts/bench/incremental-update.ts— the benchmark from perf(db): explore incremental update hot paths #1648, with the opt-in perf-tracing removed (those trace hooks only exist on that branch's instrumented code paths, so the extracted benchmark relies purely on its own wall-clock timing and runs against any revision). It measures per-write latency (median/p75/p95/min/max/stddev) plus cold hydrate time for five live-query shapes — filtered+ordered list, join, aggregate subquery, nested includes, detail view — across fixture scales (100/1k/10k rows), source index modes (none/manual), and mutation modes (synced/optimistic).scripts/bench/compare-incremental-update.ts— compares two benchmark reports and emits a markdown summary: per-case deltas with significance flags (default: >20% relative and >0.05ms absolute, so sub-ms jitter isn't flagged), plus geometric-mean ratios overall and per group to surface consistent shifts that are individually below the noise floor..github/workflows/benchmark.yml— runs on non-draft PRs that touchpackages/db,packages/db-ivm, or the benchmark itself (and onworkflow_dispatch). It benchmarks the PR merge commit and its merge-base withmainback-to-back on the same runner using the same benchmark code for both sides, then publishes the comparison as a job summary, uploads the raw reports as artifacts, and posts/updates a sticky PR comment (skipped gracefully for fork PRs, whose token is read-only — the job summary always has the results).Trigger choice
Draft PRs are skipped, so the benchmark starts when a PR is opened/marked ready for review and re-runs on pushes to it (with concurrency cancellation). Easy to tighten later (e.g. label-gated or on approval only) by changing the
on:/if:conditions.Validation
main(the suite is decoupled from perf(db): explore incremental update hot paths #1648's code changes).mainwith this tooling: overall geomean 0.86× with consistent per-group improvements at the 10k scale (down to 0.5× medians), demonstrating that real perf changes are clearly visible in the report. (It also flagged a >3× median regression for theissue detail + commentscase with optimistic writes at 10k scale on that branch — worth a look over there.)🤖 Generated with Claude Code
Summary by CodeRabbit