Skip to content

perf(ingestor): yield between committed retention batches - #2153

Merged
efiten merged 2 commits into
Kpa-clawbot:masterfrom
n30nex:codex/sqlite-retention
Oct 21, 2026
Merged

efiten merged 2 commits into
Kpa-clawbot:masterfrom
n30nex:codex/sqlite-retention

Conversation

@n30nex

@n30nex n30nex commented Oct 9, 2026 •

Copy link
Copy Markdown
Contributor

Fixes #2151. Related to #2146.

Yield to runnable goroutines after each committed full retention batch, once the writer lock is released. This gives the serial ingest consumer another opportunity to submit its next write. Batch size, cutoff and deletion semantics stay unchanged; dense fanout within one transaction remains unbounded.

The branch is rebased onto upstream b9593f71745b856651cfd6d6c80bb1f3f091e13e. The duplicate test commit 99c2722d is dropped because #2160 is already present upstream as a5b60ca5. The runtime and regression-test patch is unchanged from the previously reviewed b70a9f6b.

The raw JSON profile is omitted from this branch. The Markdown report links to the immutable original profile; the linked profile was fetched and verified against its original Git blob.

Performance scope:

  • The historical five-pair Linux comparison remains pinned to 9dbc287 versus b70a9f6, with 2,048,000 observations, 50 ingest events/s and 20 HTTP requests/s.
  • Additional-observation scheduled-to-complete p95/p99 improved by median paired ratios of 0.9503/0.9191, while service-time p99 rose 29.1% and cleanup took 0.5% longer. HTTP results were mixed, and multi-second ingestion queues remained.
  • Those figures do not measure this rebased candidate against current master. The current benchmark run is an execution smoke check, not a new performance comparison; the independent reviewer's results are not pooled into the historical study.
  • Added runtime work is one scheduler yield per full 250-transmission batch, with constant extra state and no additional SQL scans or per-row work. No config or customizer values change.

Local validation on Windows with Go 1.27.2, CGO enabled, and package parallelism limited to one:

  • Focused retention regressions passed.
  • Full ingestor suite passed.
  • Focused retention regressions under the race detector passed.
  • Both retained benchmarks passed one iteration of every fixture (typical16, dense256, singleHot).
  • The full ingestor race suite passed in 264.31 seconds with GOMAXPROCS=4. An initial run during competing test jobs reached its 600-second limit; after those jobs finished, the retry passed with the same 600-second limit and all assertions intact.

Post-push Chromium validation passed 148 checks with three explicit fixture-dependent skips. The server and frontend source are identical to upstream b9593f71; this browser pass checks the existing application journeys and does not measure retention performance. Fresh CI passed on 30743237, including the full Go job, ingestion race detector, all three browser shards and the two-architecture build.

@n30nex

n30nex commented Oct 9, 2026

Copy link
Copy Markdown
Contributor Author

Review feedback addressed (commit 323b18d)

  1. Published five paired B runs with zero errors/drops, complete retained coverage and matching 18-table digests.
  2. Reported queue-inclusive gains alongside costs: additional-observation p95/p99 improved about 5.0%/8.1%, while service-time p99 rose 29.1% and cleanup took 0.5% longer. HTTP results were mixed.
  3. Scoped every result to historical 9dbc287→b70a9f6, before later upstream changes and the optional-backend candidate. The failed prior run is excluded.

Measured report.

@A13xB0

A13xB0 commented Oct 10, 2026

Copy link
Copy Markdown
Contributor

I measured this on another machine. The gains were larger than in your CI run, and retention itself was not slower.

Benchmark. BenchmarkRetentionConcurrentIngest only calls the public PruneOldPackets, so it also runs unchanged against master. I ran it 10 times on each branch; the figures are medians of the JSON profiles.

fixture metric master this PR change
typical16 (16k tx, 256k obs) ingest completion p95 18.18 ms 7.45 ms −59.0%
queue max 3.47 ms 1.06 ms −69.5%
retention time 708.3 ms 701.5 ms −1.0%
dense256 (500 tx, 128k obs) ingest completion p95 451.2 ms 274.4 ms −39.2%
queue p95 431.2 ms 254.4 ms −41.0%
retention time 710.4 ms 706.0 ms −0.6%
singleHot (1 tx, 64k obs) ingest completion p95 1.14 ms 1.15 ms +0.6%
retention time 128.1 ms 129.0 ms +0.7%

singleHot shows no change, as you expected. That fixture is one transaction with nothing to yield between.

Not measured on staging. I'm running this PR on a staging instance, but retention only runs there once a day, so the benchmark above is the evidence.

🤖 Generated with Claude Code

https://claude.ai/code/session_01KaR4xu8gVykQBkH8yG8m6H

@efiten

efiten commented Oct 10, 2026

Copy link
Copy Markdown
Collaborator

The product change is sound. The yield runs after each batch's WriterTx has committed, so batch atomicity doesn't change, and the tests cover rollback, late arrivals and route evidence. The benchmarks from you and A13xB0 cover the perf claim.

Two things before merge:

  1. Rebase. test: make local validation independent of runner environment #2160 is merged as a5b60ca. This branch still carries the older copy of that test commit (99c2722), which conflicts with master. Please rebase onto master and drop it.
  2. Raw profile. Please drop docs/performance/sqlite-retention-b5-2026-10-09.json (1761 lines) and keep only the .md report, with a link to the CI run or your fork for the raw data. That keeps a new docs/performance/ small.

I'll merge after that.

@n30nex

n30nex commented Oct 20, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for the concrete merge requirements. I checked current upstream: the post-commit yield is still absent, so this remains useful independently of the PostgreSQL work.

The next revision should:

  1. Rebase onto current master and drop 99c2722d, since test: make local validation independent of runner environment #2160 is already merged.
  2. Remove docs/performance/sqlite-retention-b5-2026-10-09.json from this PR. Keep the Markdown report and replace its relative raw-data link with an immutable fork link or the existing Actions artifact.
  3. Keep the runtime change limited to yielding after a committed full batch, with the writer lock released. Preserve rollback, late-arrival, route-evidence and dense-fanout regressions.
  4. Validate the rebased head and, for any new performance claim, compare current master with that candidate using the same pinned harness. The October 9 figures remain historical; the service-time regressions and the single-dense-transaction limitation should stay visible.

The independent benchmark results are useful corroboration, but they should not be pooled with the differently configured paired study. The requested rebase and report cleanup have not been applied in this review pass.

@n30nex

n30nex commented Oct 20, 2026

Copy link
Copy Markdown
Contributor Author

I’m applying the requested rebase onto current upstream b9593f71, dropping the older #2160 commit and removing the raw JSON from this PR. The runtime yield change applies unchanged.

I’m preserving the original 323b18d4 revision in the fork for the historical report and raw-data link. I’ll update this branch with an explicit lease against that old head, then post the new commit and validation results.

@n30nex

n30nex commented Oct 20, 2026 •

Copy link
Copy Markdown
Contributor Author

Review feedback addressed (commit 30743237)

  1. Rebased onto upstream b9593f71 and dropped 99c2722d, retaining upstream's merged test: make local validation independent of runner environment #2160 fixes. The runtime and regression-test patch remains unchanged from b70a9f6b.
  2. Removed the raw JSON profile from the branch. The Markdown report now links to the original profile at immutable commit 323b18d4221e83c9827b4722145e2a9d5b9d9599; its downloaded bytes match the original Git blob.
  3. Kept the historical performance numbers and their service-time costs visible, and added an explicit reminder that they do not measure the rebased branch against current master. The dense-single-transaction limitation remains documented.
  4. The focused retention suite, full ingestor suite, focused race checks, full ingestor race suite and all retained benchmark fixtures passed locally. The full race suite passed in 264.31 seconds after competing test jobs finished, retaining its original 600-second limit and assertions. No new performance claim is made.

Updated report.

Post-push Chromium validation passed 148 checks with three explicit fixture-dependent skips against the unchanged upstream server/frontend. Fresh CI passed on 30743237, including the full Go job, ingestion race detector, all three browser shards and the two-architecture build. This revision is ready for review.

@efiten
efiten merged commit 93f9fc3 into Kpa-clawbot:master Oct 21, 2026
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

perf: let live ingestion run between retention batches

3 participants