perf: Avoid per-transaction Timer thread in SentryTracer (JAVA-596)#5670
perf: Avoid per-transaction Timer thread in SentryTracer (JAVA-596)#5670runningcode wants to merge 3 commits into
Conversation
📲 Install BuildsAndroid
|
Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| abfcc92 | 337.38 ms | 427.39 ms | 90.00 ms |
| 4e3e79d | 369.55 ms | 418.39 ms | 48.83 ms |
| 9054d65 | 330.94 ms | 403.24 ms | 72.30 ms |
| a416a65 | 316.52 ms | 359.67 ms | 43.15 ms |
| d15471f | 315.61 ms | 360.22 ms | 44.61 ms |
| 22f4345 | 325.23 ms | 454.66 ms | 129.43 ms |
| 6b019b7 | 403.90 ms | 546.09 ms | 142.19 ms |
| d217708 | 409.83 ms | 474.72 ms | 64.89 ms |
| 52feca7 | 314.77 ms | 378.67 ms | 63.90 ms |
| f634d01 | 375.06 ms | 420.04 ms | 44.98 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 |
| 9054d65 | 1.58 MiB | 2.29 MiB | 723.38 KiB |
| a416a65 | 1.58 MiB | 2.12 MiB | 555.26 KiB |
| d15471f | 1.58 MiB | 2.13 MiB | 559.54 KiB |
| 22f4345 | 1.58 MiB | 2.29 MiB | 719.83 KiB |
| 6b019b7 | 0 B | 0 B | 0 B |
| d217708 | 1.58 MiB | 2.10 MiB | 532.97 KiB |
| 52feca7 | 0 B | 0 B | 0 B |
| f634d01 | 1.58 MiB | 2.10 MiB | 533.40 KiB |
f823179 to
1835215
Compare
Transactions with an idle or deadline timeout each created a java.util.Timer, which spawns a thread synchronously on the calling thread (often the main thread on Android). At scale (screen loads, HTTP spans) this was the dominant source of SDK thread churn. Schedule the idle/deadline timeouts on a dedicated, shared ISentryExecutorService held by SentryOptions instead, so no thread is created per transaction. It is kept separate from the main executor so timeout callbacks (which finish transactions) don't contend with cached event sending, and it is not prewarmed: its single worker thread is spawned lazily on the first scheduled timeout and reused thereafter. The dedicated executor uses removeOnCancelPolicy so cancelled timeouts (idle timers are rescheduled per child span) don't accumulate in its queue. On finish only the scheduled futures are cancelled; the executor is closed with the SDK. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The shared timer executor introduced for transaction idle/deadline timeouts was shut down on every Scopes.close(), including SDK restart. This cancelled the pending idle timeout of any transaction started before the restart (e.g. an in-flight activity transaction), so it never auto-finished and its envelope was never sent. Only close the timer executor on a full close, not on restart, matching the pre-existing per-transaction Timer behaviour. Enable core-thread timeout on the timer executor so the instance abandoned by a restart self-terminates once idle instead of leaking a thread. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1835215 to
ccb508e
Compare
markushi
left a comment
There was a problem hiding this comment.
Nice one, looks great to me. Can't wait until we get rid of all timer tasks.
| @TestOnly | ||
| public void setTimerExecutorService(final @NotNull ISentryExecutorService timerExecutorService) { | ||
| if (timerExecutorService != null) { | ||
| this.timerExecutorService = timerExecutorService; |
There was a problem hiding this comment.
nit: I'm wondering if we should close the old/stale one, or at least mention in the docs that the existing one needs to be closed manually.
| options.getExecutorService().prewarm(); | ||
| } | ||
|
|
||
| if (options.getTimerExecutorService().isClosed()) { |
There was a problem hiding this comment.
shouldn't this be
| if (options.getTimerExecutorService().isClosed()) { | |
| if (options.getTimerExecutorService() instanceof NoOpSentryExecutorService) { |
instead?
| executorService.setRemoveOnCancelPolicy(removeOnCancelPolicy); | ||
| // let the worker thread die when idle so an executor abandoned on SDK restart (its pending | ||
| // timeouts still fire) doesn't leak a live thread once its queue drains | ||
| executorService.setKeepAliveTime(10, TimeUnit.SECONDS); |
There was a problem hiding this comment.
Any rationale why it's 10 seconds? My gut tells me a higher value (e.g. 60 seconds) could make more sense to avoid frequent restarts, but on the other hand I don't have a good way to back this argument 😅
| executorService.setRemoveOnCancelPolicy(removeOnCancelPolicy); | ||
| // let the worker thread die when idle so an executor abandoned on SDK restart (its pending | ||
| // timeouts still fire) doesn't leak a live thread once its queue drains | ||
| executorService.setKeepAliveTime(10, TimeUnit.SECONDS); |
There was a problem hiding this comment.
l: should we also pass these as ctor arguments? Because currently it's kind of implicit that if you use this ctor with removeOnCancelPolicy then these two will be set automatically for you. Or we could also just javadoc it
| // Shared executor used to schedule the timeout tasks. Null once the tracer is finished, at which | ||
| // point no more timeouts may be scheduled. It is never shut down here since it is shared | ||
| // SDK-wide. | ||
| private volatile @Nullable ISentryExecutorService timerExecutorService = null; |
There was a problem hiding this comment.
m: is there a reason to keep a ref to the service? Or could we just always retrieve it from options?
romtsn
left a comment
There was a problem hiding this comment.
couple of minor things but LGTM otherwise! great improvement
Fixes JAVA-596
Closes #5663
📜 Description
SentryTracercreated ajava.util.Timer(new Timer(true)) for every transaction configured with an idle or deadline timeout.new Timer(...)spawns a dedicated thread synchronously on the calling thread — often the main thread on Android — and each such transaction got its own throwaway thread.This change schedules the idle/deadline timeouts on a dedicated, shared
ISentryExecutorServiceinstead:SentryOptions.getTimerExecutorService()— a singleSentryExecutorServicecreated inactivate(), kept separate from the main executor so timeout callbacks (which finish transactions) don't contend with cached-event sending.SentryTracernow uses cancellableFutures (idleTimeoutFuture/deadlineTimeoutFuture) scheduled on that executor. On finish only the futures are cancelled; the executor is closed with the SDK (Scopes.close) and recreated on re-init if it was closed.💡 Motivation and Context
At scale (screen loads, HTTP spans, user-interaction transactions) the per-transaction
Timerwas the dominant source of SDK thread churn, and the thread creation happened on the caller's thread. This is item #4 ("Note B") of the thread/executor audit (JAVA-570 / SDK-1347) and is the biggest thread-count win in that effort. Using a dedicated executor (rather than the main one) keeps transaction-finishing work off the executor that sends cached events.💚 How did you test it?
SentryTracerTestidle/deadline coverage to the newFuture-based scheduling; the "timer executor shut down" cases now close the dedicated executor to exercise the immediate-finish fallback.ActivityLifecycleIntegrationTestidle-timeout test now installs a realtimerExecutorService.:sentry:testand:sentry-android-core:testDebugUnitTestpass.📝 Checklist
sendDefaultPIIis enabled.🔮 Next steps
Follow-up audit items can fold the remaining per-
Timersites (DefaultCompositePerformanceCollector,RateLimiter,LifecycleWatcher) onto the same dedicated scheduler in a separate cleanup pass.