perf(core): Remove executor prewarm#5681
Open
runningcode wants to merge 1 commit into
Open
Conversation
e4b7ee0 to
c219919
Compare
📲 Install BuildsAndroid
|
Contributor
Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| abfcc92 | 304.04 ms | 370.33 ms | 66.29 ms |
| 4e3e79d | 365.83 ms | 477.62 ms | 111.79 ms |
| d8912da | 329.94 ms | 389.68 ms | 59.74 ms |
| d15471f | 294.13 ms | 399.49 ms | 105.36 ms |
| abf451a | 332.82 ms | 403.67 ms | 70.85 ms |
| 604a261 | 380.65 ms | 451.27 ms | 70.62 ms |
| 806307f | 357.85 ms | 424.64 ms | 66.79 ms |
| 22f4345 | 307.87 ms | 354.51 ms | 46.64 ms |
| d217708 | 375.27 ms | 415.68 ms | 40.41 ms |
| c3ee041 | 310.64 ms | 361.90 ms | 51.26 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| abfcc92 | 1.58 MiB | 2.13 MiB | 557.31 KiB |
| 4e3e79d | 0 B | 0 B | 0 B |
| d8912da | 0 B | 0 B | 0 B |
| d15471f | 1.58 MiB | 2.13 MiB | 559.54 KiB |
| abf451a | 1.58 MiB | 2.20 MiB | 635.29 KiB |
| 604a261 | 1.58 MiB | 2.10 MiB | 533.42 KiB |
| 806307f | 1.58 MiB | 2.10 MiB | 533.42 KiB |
| 22f4345 | 1.58 MiB | 2.29 MiB | 719.83 KiB |
| d217708 | 1.58 MiB | 2.10 MiB | 532.97 KiB |
| c3ee041 | 0 B | 0 B | 0 B |
c49b77c to
9875242
Compare
SentryExecutorService is single-threaded, and prewarm() is submitted ahead of loadLazyFields() during init, so its 40-task schedule/cancel/purge loop cannot reduce first-task latency — it can only delay it. The thread creation and executor class loading it aimed to warm are paid identically by the first real task (loadLazyFields), which is submitted unconditionally right after, so prewarm warms nothing that would not already be warmed. On-device A/B benchmarks on a Galaxy A55 (Android 16) show no measurable first-useful-task speedup from prewarm and ~20us of extra background-thread work. Remove prewarm() from ISentryExecutorService and its implementations and from both init call sites. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
9875242 to
5ff1185
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
prewarm()scheduled 40 dummy tasks far in the future, cancelled them, and purged the queue, in order to pre-grow the single-threadedScheduledThreadPoolExecutor's work queue duringSentry.init. On-device measurement shows the cost it avoids is negligible-to-zero, so this removes it (and the now-unused constants) along with its two init call sites.Why it's safe to remove
prewarm's only job was to pre-grow the executor's work queue so a later resize wouldn't happen "in an unexpected area of the SDK". Measured on a Galaxy A55 (Android 16), growing the queue the full 16→54 (all three array resizes) costs ~8µs total — and that is the ceiling: init only ever queues a handful of tasks, well under the default 16-slot capacity, so in practice no resize happens and the saving is 0µs.
PrewarmBenchmarkTest, n=1000 × 3 runs, median µs. Both paths schedule 40 tasks into an empty queue (identical per-insert work); they differ only in whether the backing array resizes:It also can't help, and slightly regresses init
The executor is single-threaded and prewarm is submitted ahead of the first useful task (
loadLazyFields, submitted right after inSentry.init), so the 40-task loop can only delay that task, never speed it up. Thread creation and executor class-loading are paid by whichever task runs first, soloadLazyFieldswarms them identically with or without prewarm.Measured end-to-end, the time until the first useful task finishes (from a fresh executor, n=400 × 3 runs) is ~100µs slower with prewarm than without, at every percentile; a plain
submit(noop)is indistinguishable from doing nothing:(
PrewarmBenchmarkTestalso records the per-op loop cost and the process-once class-loading cost, which is unavoidable regardless of prewarm; both confirm the above.)Changes
prewarm()fromISentryExecutorService,SentryExecutorService(and the now-unusedINITIAL_QUEUE_SIZE/dummyRunnable), andNoOpSentryExecutorService.Sentry.initre-init branch andSentryOptions.activate()).PrewarmBenchmarkTest.Original prewarm PR: #4606 — its only stated reason was the queue-growth cost measured above.
🤖 Generated with Claude Code