fix(db): avoid full row origin snapshots#1640
Conversation
📝 WalkthroughWalkthrough
ChangesBulk mutation merge update
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
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: |
|
We hit the same root cause in a parallel investigation (profiled the two One additional, independent hotspot we found along the way that this PR doesn't cover: Problem/cause: Fix: merge through a Commit: kevin-dp/db@5b900cd (on our investigation branch, |
applyMutations did a findIndex scan over the existing mutations array for every incoming mutation, making bulk operations quadratic: a single insert() of 50k rows spent most of its time in this scan. Merge through a globalKey-keyed Map instead, preserving insertion order and rebuilding the array in place to keep its identity for external holders. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/db/src/transactions.ts (1)
336-370: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding a regression test for bulk-merge ordering/semantics.
This function's correctness now hinges on subtle
Mapinsertion-order behavior (replace keeps position, delete-then-reinsert moves to end) across insert/update/delete combinations. A dedicated test exercising bulk mutations with repeatedglobalKeys (including cancel-out and reorder cases) would guard against regressions in this performance-critical path.🤖 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 `@packages/db/src/transactions.ts` around lines 336 - 370, Add a regression test for Transactions.applyMutations that covers bulk mutation merging with repeated globalKey values, including update/replace, delete-cancels-insert, and delete-then-reinsert ordering cases. Exercise the applyMutations method directly with a sequence of PendingMutation entries and assert both the final mutations contents and their preserved order to lock in the current Map-based semantics.
🤖 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.
Nitpick comments:
In `@packages/db/src/transactions.ts`:
- Around line 336-370: Add a regression test for Transactions.applyMutations
that covers bulk mutation merging with repeated globalKey values, including
update/replace, delete-cancels-insert, and delete-then-reinsert ordering cases.
Exercise the applyMutations method directly with a sequence of PendingMutation
entries and assert both the final mutations contents and their preserved order
to lock in the current Map-based semantics.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4c92499c-8979-428c-8293-cd914e4e6bfd
📒 Files selected for processing (2)
.changeset/quiet-row-origins.mdpackages/db/src/transactions.ts
✅ Files skipped from review due to trivial changes (1)
- .changeset/quiet-row-origins.md
Summary
Fixes two collection/transaction performance hot paths:
rowOriginssnapshots during incremental collection updates.Transaction.applyMutationsmerge bulk mutations in O(n) instead of O(n²).Root Cause
Single-row incremental writes were cloning the full
rowOriginsmap before propagating changes. SincerowOriginsgrows with the source collection, normal incremental writes paid O(source collection size) work before the query graph could process the actual changed keys.Kevin also identified a separate bulk mutation issue:
Transaction.applyMutationsusedfindIndexfor every incoming mutation, making large bulk operations quadratic.Changes
rowOriginsmap during optimistic recompute, where confirmed origins are not mutated.globalKey-keyedMap, then rebuild the public mutations array in place to preserve identity.Validation
pnpm --dir packages/db exec tsc --noEmitpnpm --dir packages/db exec vitest --run tests/transactions.test.ts tests/collection-subscribe-changes.test.ts tests/collection-change-events.test.ts tests/collection-truncate.test.ts