-
-
Notifications
You must be signed in to change notification settings - Fork 475
perf: Avoid per-transaction Timer thread in SentryTracer (JAVA-596) #5670
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. Weโll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -49,6 +49,17 @@ public SentryExecutorService(final @Nullable SentryOptions options) { | |
| this(new ScheduledThreadPoolExecutor(1, new SentryExecutorServiceThreadFactory()), options); | ||
| } | ||
|
|
||
| SentryExecutorService(final @Nullable SentryOptions options, final boolean removeOnCancelPolicy) { | ||
| this(options); | ||
| // removes cancelled tasks from the work queue immediately instead of leaving them until their | ||
| // scheduled time; useful for executors that frequently reschedule (e.g. transaction timeouts) | ||
| 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); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 ๐
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| executorService.allowCoreThreadTimeOut(true); | ||
| } | ||
|
|
||
| public SentryExecutorService() { | ||
| this(new ScheduledThreadPoolExecutor(1, new SentryExecutorServiceThreadFactory()), null); | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -317,6 +317,14 @@ public class SentryOptions { | |
| /** Sentry Executor Service that sends cached events and envelopes on App. start. */ | ||
| private @NotNull ISentryExecutorService executorService = NoOpSentryExecutorService.getInstance(); | ||
|
|
||
| /** | ||
| * Dedicated executor for scheduling transaction idle/deadline timeouts. Kept separate from {@link | ||
| * #executorService} so timeout callbacks (which finish transactions) don't contend with cached | ||
| * event sending. | ||
| */ | ||
| private @NotNull ISentryExecutorService timerExecutorService = | ||
| NoOpSentryExecutorService.getInstance(); | ||
|
|
||
| /** | ||
| * Whether SpotlightIntegration has already been loaded via reflection. This prevents re-adding it | ||
| * if the user removed it in their configuration callback and activate() is called again. | ||
|
|
@@ -681,6 +689,13 @@ public void activate() { | |
| executorService.prewarm(); | ||
| } | ||
|
|
||
| if (timerExecutorService instanceof NoOpSentryExecutorService) { | ||
| // Not prewarmed: its single worker thread is spawned lazily on the first scheduled timeout | ||
| // and then reused across all transactions. removeOnCancelPolicy keeps the work queue from | ||
| // accumulating cancelled timeouts (idle timers are cancelled and rescheduled per child span). | ||
| timerExecutorService = new SentryExecutorService(this, true); | ||
| } | ||
|
|
||
| // SpotlightIntegration is loaded via reflection to allow the sentry-spotlight module | ||
| // to be excluded from release builds, preventing insecure HTTP URLs from appearing in APKs. | ||
| // Only attempt once to avoid re-adding after user removal in their configuration callback. | ||
|
|
@@ -1568,6 +1583,30 @@ public void setExecutorService(final @NotNull ISentryExecutorService executorSer | |
| } | ||
| } | ||
|
|
||
| /** | ||
| * Returns the dedicated executor used to schedule transaction idle/deadline timeouts. | ||
| * | ||
| * @return the timer executor service | ||
| */ | ||
| @ApiStatus.Internal | ||
| @NotNull | ||
| public ISentryExecutorService getTimerExecutorService() { | ||
| return timerExecutorService; | ||
| } | ||
|
|
||
| /** | ||
| * Sets the dedicated executor used to schedule transaction idle/deadline timeouts. | ||
| * | ||
| * @param timerExecutorService the timer executor service | ||
| */ | ||
| @ApiStatus.Internal | ||
| @TestOnly | ||
| public void setTimerExecutorService(final @NotNull ISentryExecutorService timerExecutorService) { | ||
| if (timerExecutorService != null) { | ||
| this.timerExecutorService = timerExecutorService; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Returns the connection timeout in milliseconds. | ||
| * | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -12,9 +12,8 @@ | |
| import java.util.List; | ||
| import java.util.ListIterator; | ||
| import java.util.Map; | ||
| import java.util.Timer; | ||
| import java.util.TimerTask; | ||
| import java.util.concurrent.CopyOnWriteArrayList; | ||
| import java.util.concurrent.Future; | ||
| import java.util.concurrent.atomic.AtomicBoolean; | ||
| import java.util.concurrent.atomic.AtomicReference; | ||
| import org.jetbrains.annotations.ApiStatus; | ||
|
|
@@ -37,10 +36,13 @@ public final class SentryTracer implements ITransaction { | |
| */ | ||
| private @NotNull FinishStatus finishStatus = FinishStatus.NOT_FINISHED; | ||
|
|
||
| private volatile @Nullable TimerTask idleTimeoutTask; | ||
| private volatile @Nullable TimerTask deadlineTimeoutTask; | ||
| private volatile @Nullable Future<?> idleTimeoutFuture; | ||
| private volatile @Nullable Future<?> deadlineTimeoutFuture; | ||
|
|
||
| private volatile @Nullable Timer timer = null; | ||
| // 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; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| private final @NotNull AutoClosableReentrantLock timerLock = new AutoClosableReentrantLock(); | ||
| private final @NotNull AutoClosableReentrantLock tracerLock = new AutoClosableReentrantLock(); | ||
|
|
||
|
|
@@ -99,7 +101,7 @@ public SentryTracer( | |
|
|
||
| if (transactionOptions.getIdleTimeout() != null | ||
| || transactionOptions.getDeadlineTimeout() != null) { | ||
| timer = new Timer(true); | ||
| timerExecutorService = scopes.getOptions().getTimerExecutorService(); | ||
|
|
||
| scheduleDeadlineTimeout(); | ||
| scheduleFinish(); | ||
|
|
@@ -109,22 +111,16 @@ public SentryTracer( | |
| @Override | ||
| public void scheduleFinish() { | ||
| try (final @NotNull ISentryLifecycleToken ignored = timerLock.acquire()) { | ||
| if (timer != null) { | ||
| if (timerExecutorService != null) { | ||
| final @Nullable Long idleTimeout = transactionOptions.getIdleTimeout(); | ||
|
|
||
| if (idleTimeout != null) { | ||
| cancelIdleTimer(); | ||
| isIdleFinishTimerRunning.set(true); | ||
| idleTimeoutTask = | ||
| new TimerTask() { | ||
| @Override | ||
| public void run() { | ||
| onIdleTimeoutReached(); | ||
| } | ||
| }; | ||
|
|
||
| try { | ||
| timer.schedule(idleTimeoutTask, idleTimeout); | ||
| idleTimeoutFuture = | ||
| timerExecutorService.schedule(this::onIdleTimeoutReached, idleTimeout); | ||
| } catch (Throwable e) { | ||
| scopes | ||
| .getOptions() | ||
|
|
@@ -265,13 +261,12 @@ public void finish( | |
| }); | ||
| final SentryTransaction transaction = new SentryTransaction(this); | ||
|
|
||
| if (timer != null) { | ||
| if (timerExecutorService != null) { | ||
| try (final @NotNull ISentryLifecycleToken ignored = timerLock.acquire()) { | ||
| if (timer != null) { | ||
| if (timerExecutorService != null) { | ||
| cancelIdleTimer(); | ||
| cancelDeadlineTimer(); | ||
| timer.cancel(); | ||
| timer = null; | ||
| timerExecutorService = null; | ||
| } | ||
| } | ||
| } | ||
|
|
@@ -295,10 +290,10 @@ public void finish( | |
|
|
||
| private void cancelIdleTimer() { | ||
| try (final @NotNull ISentryLifecycleToken ignored = timerLock.acquire()) { | ||
| if (idleTimeoutTask != null) { | ||
| idleTimeoutTask.cancel(); | ||
| if (idleTimeoutFuture != null) { | ||
| idleTimeoutFuture.cancel(false); | ||
| isIdleFinishTimerRunning.set(false); | ||
| idleTimeoutTask = null; | ||
| idleTimeoutFuture = null; | ||
| } | ||
| } | ||
| } | ||
|
|
@@ -307,18 +302,12 @@ private void scheduleDeadlineTimeout() { | |
| final @Nullable Long deadlineTimeOut = transactionOptions.getDeadlineTimeout(); | ||
| if (deadlineTimeOut != null) { | ||
| try (final @NotNull ISentryLifecycleToken ignored = timerLock.acquire()) { | ||
| if (timer != null) { | ||
| if (timerExecutorService != null) { | ||
| cancelDeadlineTimer(); | ||
| isDeadlineTimerRunning.set(true); | ||
| deadlineTimeoutTask = | ||
| new TimerTask() { | ||
| @Override | ||
| public void run() { | ||
| onDeadlineTimeoutReached(); | ||
| } | ||
| }; | ||
| try { | ||
| timer.schedule(deadlineTimeoutTask, deadlineTimeOut); | ||
| deadlineTimeoutFuture = | ||
| timerExecutorService.schedule(this::onDeadlineTimeoutReached, deadlineTimeOut); | ||
| } catch (Throwable e) { | ||
| scopes | ||
| .getOptions() | ||
|
|
@@ -335,10 +324,10 @@ public void run() { | |
|
|
||
| private void cancelDeadlineTimer() { | ||
| try (final @NotNull ISentryLifecycleToken ignored = timerLock.acquire()) { | ||
| if (deadlineTimeoutTask != null) { | ||
| deadlineTimeoutTask.cancel(); | ||
| if (deadlineTimeoutFuture != null) { | ||
| deadlineTimeoutFuture.cancel(false); | ||
| isDeadlineTimerRunning.set(false); | ||
| deadlineTimeoutTask = null; | ||
| deadlineTimeoutFuture = null; | ||
| } | ||
| } | ||
| } | ||
|
|
@@ -973,20 +962,20 @@ Span getRoot() { | |
|
|
||
| @TestOnly | ||
| @Nullable | ||
| TimerTask getIdleTimeoutTask() { | ||
| return idleTimeoutTask; | ||
| Future<?> getIdleTimeoutFuture() { | ||
| return idleTimeoutFuture; | ||
| } | ||
|
|
||
| @TestOnly | ||
| @Nullable | ||
| TimerTask getDeadlineTimeoutTask() { | ||
| return deadlineTimeoutTask; | ||
| Future<?> getDeadlineTimeoutFuture() { | ||
| return deadlineTimeoutFuture; | ||
| } | ||
|
|
||
| @TestOnly | ||
| @Nullable | ||
| Timer getTimer() { | ||
| return timer; | ||
| ISentryExecutorService getTimerExecutorService() { | ||
| return timerExecutorService; | ||
| } | ||
|
|
||
| @TestOnly | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't this be
instead?