perf(core): Batch and coalesce scope-persistence disk writes - #5791
Conversation
📲 Install BuildsAndroid
|
Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 6b019b7 | 343.31 ms | 417.23 ms | 73.91 ms |
| d15471f | 342.08 ms | 415.44 ms | 73.35 ms |
| 8687935 | 332.52 ms | 362.23 ms | 29.71 ms |
| 5b1a06b | 352.27 ms | 413.70 ms | 61.43 ms |
| 91bb874 | 314.47 ms | 440.00 ms | 125.53 ms |
| 0ee65e9 | 321.06 ms | 361.24 ms | 40.18 ms |
| e63ad34 | 323.67 ms | 390.33 ms | 66.67 ms |
| 33a08cc | 267.08 ms | 340.45 ms | 73.37 ms |
| 27d7cf8 | 397.90 ms | 498.65 ms | 100.75 ms |
| ee747ae | 405.43 ms | 485.70 ms | 80.28 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 6b019b7 | 0 B | 0 B | 0 B |
| d15471f | 1.58 MiB | 2.13 MiB | 559.54 KiB |
| 8687935 | 1.58 MiB | 2.19 MiB | 619.17 KiB |
| 5b1a06b | 0 B | 0 B | 0 B |
| 91bb874 | 1.58 MiB | 2.13 MiB | 559.07 KiB |
| 0ee65e9 | 0 B | 0 B | 0 B |
| e63ad34 | 0 B | 0 B | 0 B |
| 33a08cc | 1.58 MiB | 2.12 MiB | 555.28 KiB |
| 27d7cf8 | 1.58 MiB | 2.12 MiB | 549.42 KiB |
| ee747ae | 1.58 MiB | 2.10 MiB | 530.95 KiB |
Previous results on branch: no/java-628-batch-scope-persistence
Startup times
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 9a85552 | 315.94 ms | 370.50 ms | 54.56 ms |
| 3376dc7 | 322.45 ms | 374.02 ms | 51.57 ms |
| 5ad5c95 | 318.13 ms | 364.98 ms | 46.85 ms |
| 8e8cb90 | 313.06 ms | 356.54 ms | 43.48 ms |
| 47395a5 | 312.85 ms | 362.90 ms | 50.05 ms |
| 1afaf81 | 334.74 ms | 380.29 ms | 45.55 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 9a85552 | 0 B | 0 B | 0 B |
| 3376dc7 | 0 B | 0 B | 0 B |
| 5ad5c95 | 0 B | 0 B | 0 B |
| 8e8cb90 | 0 B | 0 B | 0 B |
| 47395a5 | 0 B | 0 B | 0 B |
| 1afaf81 | 0 B | 0 B | 0 B |
cc7d1be to
73afe6e
Compare
There was a problem hiding this comment.
Thanks for this @runningcode! A few comments for your consideration.
And two big-picture thoughts...
Thread-safety:
I see that QueueFile isn't thread-safe. Worth double-checking that we don't expect the buffered write patterns to aggravate any concurrency concerns that were already there but perhaps less common (totally hand-wavy on my part – just flagging that I haven't done a thread-safety deep dive in my review).
Testing / monitoring:
Given that this is core infrastructure, do you plan to perform any manual tests / add tests to the sample app, etc.?
runningcode
left a comment
There was a problem hiding this comment.
Thanks for the thorough review, @0xadam-brown ! I've replied to your points and fixed the rest in the PR.
7396a46 to
54db93b
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 54db93b. Configure here.
romtsn
left a comment
There was a problem hiding this comment.
couple of minor things, but looks good, great improvement!
Scope persistence wrote to disk on every scope mutation, which dominated SDK cost during startup: each breadcrumb triggered a synchronous fsync'd QueueFile append, and every other scope field (contexts, trace, user, tags, ...) rewrote its whole file on each change even though only the latest value matters. Coalesce mutations instead of writing eagerly. Each field keeps only its latest pending value and is flushed once per debounce window; breadcrumbs are buffered and appended together behind a single fsync (QueueFile gains an opt-in buffered-write mode plus sync()). This trades a small data-loss window (~100ms before the process dies) for far fewer writes and fsyncs. Persistence exists to enrich crash/ANR events on the next launch, and was already asynchronous, so the widened loss window is acceptable.
…628) Scope-persistence flushes were debounced 100ms behind a scheduled task. The debounce was unnecessary: the Sentry executor is single-threaded, so a submitted flush task already sits in the queue long enough for mutations arriving behind it to be folded into the same write. That is exactly the window that matters, since the queue is deepest during startup. Submit the flush instead of scheduling it. Coalescing now tracks executor load rather than a fixed delay, which closes the data-loss window the debounce introduced. Guard against the executor rejecting the task without throwing once its queue is full, which would otherwise leave the pending flag set and stop scope persistence for the rest of the process. Also record why resetCache() deliberately leaves pending mutations alone: they only ever hold values from the current process, so dropping them would lose scope state set during init rather than clearing the previous run's data. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Batching split one ordered stream of breadcrumb operations into two independent pieces of state: a queue of pending adds and a separate clear flag. Independent state cannot preserve an ordering that the old FIFO executor queue gave for free. flushPending consumed the clear flag with a CAS at the top, then spent the rest of the method serializing and writing each breadcrumb. A clear plus a subsequent add landing in that window had its clear applied after the new breadcrumb was already on disk, so the follow-up flush wiped a breadcrumb that was added after the clear. Breadcrumbs are added from arbitrary threads while the flush runs on the executor thread, so this needs no unusual timing. Enqueue the clear into the breadcrumb queue as a sentinel instead, mirroring the DELETE_MARKER pattern already used for pendingWrites. Ordering becomes intrinsic to the queue rather than something the flush has to reconstruct. This also stops setBreadcrumbs(emptyList()) from calling clear() on the shared queue, which could discard a breadcrumb offered concurrently by another thread even with no flush in flight.
"Durable" overstated what the buffered-write mode guarantees. sync() issues an fsync, but durability also depends on the storage stack, so describe what the code does — write to disk — rather than promise an outcome it cannot ensure on its own. Also name the pendingBreadcrumbs queue for what it holds: requests to add and clear, not breadcrumbs already buffered. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
flushPending existed only so the @testonly flush() could do the disk write without the latch bookkeeping that belongs to the queued task. Nothing in production called flush(), and its only test asserted the behaviour of flush() itself, so the second entry point was buying two near-identical methods and a way to corrupt hasPendingFlush. Drop flush() and merge the pair: flush() is now the queued task and writePending() the write it performs. Clear hasPendingFlush in a finally so an unexpected throw cannot leave the flag set, which would stop scope persistence for the rest of the process. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
bufferedWritesSurviveReopenAfterSync was testAddOneElement with a second element and a sync() call: same write, close, reopen, read-it-back shape, and testAddAndRemoveElements already covers round-tripping far more thoroughly. The sync() it appeared to exercise was not actually load-bearing. close() flushes through to the OS whichever mode the file was opened in, so the assertions would hold even if sync() did nothing — the name promised durability semantics the test could not verify in-process. Verifying that would take a killed process, not a reopen. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Square archived Tape on 2024-10-25, so the upstream link in each file header points at a snapshot that will never receive fixes. Our copy has also drifted from it: corruption recovery, a bounded ring size, and optional buffered writes. Say both things where a reader will look — the file headers, the class javadoc, and THIRD_PARTY_NOTICES.md — so nobody diffs against upstream expecting it to explain our behaviour, or files a bug there. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
writePending() copied the key set into an ArrayList before draining it, which allocates on every flush for no benefit: ConcurrentHashMap's iterator is weakly consistent, so walking it while removing entries is already safe. Removal still goes through the map, not the iterator, because that is an atomic get-and-remove — we never drop a value stored by a mutation racing with the loop. Keys added after iteration starts can be missed, which is fine since flush() re-checks and queues another write. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The 8.51.0 release renamed the Unreleased heading this entry was written under, so rebasing onto main left it inside the released 8.51.0 notes describing a change that did not ship in it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
038401d to
b981e82
Compare

Summary
Scope persistence wrote to disk on every scope mutation, which a customer cold-start trace (JAVA-628) showed to be the single largest SDK cost during startup — larger than init itself. On the Sentry executor thread, during a 1.7 s startup window:
FileObjectQueue#add×107 — every breadcrumb was a synchronous, fsync'dQueueFileappend (the file was opened inrwdmode).PersistingScopeObserver#store×310 —setContexts/setTrace/setUser/setTags/… each rewrote their whole file on every change, even though only the latest value matters.JsonSerializer#serialize×421.(The trace was captured with method-level tracing, which inflates per-slice durations; the call counts are the reliable signal, so the wall-clock attributions from the ticket are omitted here.)
I did a benchmark to compare the performance improvement of this PR and an alternative.

Tape as shipped is what we have on
main, tape without fsync is this PR and JSONL is replacing tape with a single json file.What changed
Instead of writing eagerly on each mutation, mutations are recorded as pending state and flushed by a single task submitted to the Sentry executor:
QueueFilegains an opt-in buffered-write mode (synchronousWrites(false)) plus async()method; only the breadcrumb queue opts in, the ANR-profile queues keep synchronous writes.CLEAR_MARKERsentinel, mirroring the existingDELETE_MARKERfor field writes) rather than a list plus a separate flag, so a clear cannot wipe a breadcrumb added after it. Batching split what used to be one ordered stream of executor tasks into independent state; keeping both in one queue makes the ordering intrinsic again.Tradeoff
Persistence exists to enrich crash/ANR events on the next launch and was already asynchronous, so nothing changes about the "data is only needed if the process dies" contract. Because the flush is submitted rather than delayed, pending state is written as soon as the executor drains to it — there is no added time-based loss window. On-disk format is unchanged; restore is unaffected.
Notes
enableScopePersistenceoption; no new public option.requestFlush()guards against the executor rejecting the flush task.SentryExecutorServicereturns aCancelledFutureinstead of throwing once its queue is full, which would otherwise leave the pending flag set and silently stop scope persistence for the rest of the process.resetCache()deliberately leaves pending mutations alone: they only ever hold values from the current process, so dropping them would lose scope state set during init rather than clearing the previous run's data.Scopes#addBreadcrumbcall (9.3 ms of it insidePersistingScopeObserver#addBreadcrumb, suspected lazyFileObjectQueueinit). That is intentionally not addressed here.🤖 Generated with Claude Code