ADFA-4332: Batch JVM-symbol removals into one transaction (N+1)#1411
ADFA-4332: Batch JVM-symbol removals into one transaction (N+1)#1411fryanpan wants to merge 3 commits into
Conversation
|
Warning Review limit reached
More reviews will be available in 27 minutes and 34 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
✨ 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 |
Sentry APPDEVFORALL-SE. Batched primitive (single transaction, chunked IN) to collapse the per-file DELETE FROM jvm_symbols N+1. Call-site wiring TODO. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Completes the call-site wiring left TODO by the primitive commit. IndexWorker now coalesces a run of consecutive RemoveFromIndex commands into a single JvmSymbolIndex.removeBySources call (one SQLite transaction) instead of one DELETE FROM jvm_symbols per file (the N+1, Sentry APPDEVFORALL-SE). WorkerQueue gains a non-blocking pollIndexQueue + single-slot pushBack so a non-removal command polled while draining is returned in order, not dropped. IndexWorkerBatchRemovalTest is failing-first: it asserts N removals issue ONE batched transaction (removeBySourcesBatches==1, removeBySourceCalls==0) and verifies InMemory/SQLite-style parity. Reverting applyRemovals to the per-file loop turns the two wiring tests red (expected 1 but was 0). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
fryanpan
force-pushed
the
ADFA-4332-batched-removebysources-jvm-symbols
branch
from
b3d331a to
5959be0
Compare
| // Per-file metadata index has no batch API; remove individually. | ||
| for (path in paths) { | ||
| fileIndex.remove(path) | ||
| } |
There was a problem hiding this comment.
The KtFileMetadataIndex is a wrapper over an Index, so I think we could add a similar API to KtFileMetadataIndex for batch removal.
2 participants
Jira Ticket: https://appdevforall.atlassian.net/browse/ADFA-4332
Sentry Issue: https://appdevforall-inc-9p.sentry.io/issues/APPDEVFORALL-SE
Reproduction Details
A performance (N+1) issue, not a crash. Index symbol cleanup issued one
DELETE FROM jvm_symbols WHERE _source_id = ?transaction per file instead of one batched transaction, producing a long stall during editor load/indexing. Sentry shows a 17.9-secondEditorActivityKtui.loadspan dominated by the per-file deletes.Stack Trace
No exception (performance issue). Offending span:
(device.class: high — i.e. not limited to low-end hardware)
User Steps
User steps leading up to crash, based on Sentry breadcrumbs:
createSearchScopes(...)×N) and removes/re-inserts JVM symbols per source file — the per-fileDELETEloop is what inflates the load span.Was able to reproduce in a unit test?
Yes.
IndexWorkerBatchRemovalTest(:lsp:kotlin) asserts N consecutive removals collapse into exactly ONE batchedremoveBySourcestransaction via a counting Index decorator. Baseline (per-file loop): FAILS ("expected: 1 but was: 0"). Branch: 4/4 pass.What Was Fixed
The shipped commit added the batched
removeBySourcesprimitive (single transaction, chunkedIN (...)under the 999-param limit, InMemory/SQLite parity) but had no callers. This PR wires it in:IndexWorkercoalesces a run of consecutiveRemoveFromIndexcommands into oneremoveBySourcescall;WorkerQueuegains non-blockingpollIndexQueue()+ single-slotpushBackIndexQueue()so a non-removal command polled while draining is returned in order.Partial: the ticket also mentions wrapping per-file remove+insert in one transaction in
SourceFileIndexer— NOT wired (needs a new transactional index API); it is not the N+1 source (this site is one remove + one insert per file — an atomicity/correctness concern, not the throughput hang). Filed as follow-up ADFA-4359 (https://appdevforall.atlassian.net/browse/ADFA-4359): the remove and insert run as two separate transactions with the slow, cancellable, often-throwinganalyzepass between them, so a cancel/throw after the remove but before the insert drops the file's symbols until a full re-index — fix is a new atomicreplaceBySourceindex API (SQLite + InMemory) + analyze-first reorder.Testing
:lsp:kotlin:testV8DebugUnitTest --tests …IndexWorkerBatchRemovalTest→ 4/4 green (red on the pre-fix per-file loop). Local CodeRabbit review: no findings. Reviewer note (local): the newWorkerQueuepoll/pushback glue is itself untested — add a directWorkerQueueordering test before merge.Fixes APPDEVFORALL-SE