Skip to content

Commit 24bcfed

Browse files
markushiclaude
andcommitted
fix(profiler): Address Perfetto profiling PR review feedback
- Re-check shouldStop in async chunk callback to avoid an extra chunk when stop is requested while the callback is pending - Use a lambda instead of a method ref for meta_length (Android desugaring) - Remove unused getStopFuture()/getActiveTraceCount() test hooks - Send chunks via the profiling executor, running inline when already on its thread to avoid self-submitting into the single-threaded executor - Use 101Hz sampling to avoid lockstep with the display refresh rate Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 parent 6e522f6 commit 24bcfed

5 files changed

Lines changed: 68 additions & 26 deletions

File tree

sentry-android-core/api/sentry-android-core.api

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -341,7 +341,6 @@ public final class io/sentry/android/core/NetworkBreadcrumbsIntegration : io/sen
341341
public class io/sentry/android/core/PerfettoContinuousProfiler : io/sentry/IContinuousProfiler, io/sentry/transport/RateLimiter$IRateLimitObserver {
342342
public fun <init> (Lio/sentry/ILogger;Lio/sentry/android/core/internal/util/SentryFrameMetricsCollector;Lio/sentry/util/LazyEvaluator$Evaluator;Ljava/util/function/Supplier;)V
343343
public fun close (Z)V
344-
public fun getActiveTraceCount ()I
345344
public fun getChunkId ()Lio/sentry/protocol/SentryId;
346345
public fun getProfilerId ()Lio/sentry/protocol/SentryId;
347346
public fun isRunning ()Z

sentry-android-core/src/main/java/io/sentry/android/core/PerfettoContinuousProfiler.java

Lines changed: 22 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -71,6 +71,10 @@ public class PerfettoContinuousProfiler
7171
implements IContinuousProfiler, RateLimiter.IRateLimitObserver {
7272
private static final long MAX_CHUNK_DURATION_MILLIS = 60000;
7373

74+
// Matches the thread name produced by SentryExecutorService's thread factory, used to detect
75+
// when we are already running on the executor thread.
76+
private static final String EXECUTOR_THREAD_NAME_PREFIX = "SentryExecutorServiceThreadFactory";
77+
7478
private final @NotNull ILogger logger;
7579
private final @NotNull LazyEvaluator.Evaluator<ISentryExecutorService> executorServiceSupplier;
7680
private final @NotNull Supplier<PerfettoProfiler> perfettoProfilerFactory;
@@ -379,10 +383,12 @@ private void onChunkCollected(
379383

380384
if (shouldRestart) {
381385
try (final @NotNull ISentryLifecycleToken ignored = lock.acquire()) {
382-
if (isRunning || isClosed.get()) {
386+
// shouldStop is re-checked here (not just at capture time) because a stopProfiler() or
387+
// close() may have been requested while this async callback was pending.
388+
if (isRunning || isClosed.get() || shouldStop) {
383389
logger.log(
384390
SentryLevel.DEBUG,
385-
"Profile chunk finished, but profiler was already restarted or closed. Skipping.");
391+
"Profile chunk finished, but profiler was already restarted, closed or stopped. Skipping.");
386392
return;
387393
}
388394
logger.log(SentryLevel.DEBUG, "Profile chunk finished. Starting a new one.");
@@ -403,32 +409,26 @@ private void sendChunk(
403409
final @NotNull ProfileChunk.Builder builder,
404410
final @NotNull IScopes scopes,
405411
final @NotNull SentryOptions options) {
412+
final @NotNull Runnable task =
413+
() -> {
414+
if (isClosed.get()) {
415+
return;
416+
}
417+
scopes.captureProfileChunk(builder.build(options));
418+
};
406419
try {
407-
options
408-
.getExecutorService()
409-
.submit(
410-
() -> {
411-
if (isClosed.get()) {
412-
return;
413-
}
414-
scopes.captureProfileChunk(builder.build(options));
415-
});
420+
// The chunk timer callback (stopInternal) already runs on the executor thread; submitting
421+
// back into the same single-threaded executor from there can deadlock, so run inline instead.
422+
if (Thread.currentThread().getName().startsWith(EXECUTOR_THREAD_NAME_PREFIX)) {
423+
task.run();
424+
} else {
425+
executorServiceSupplier.evaluate().submit(task);
426+
}
416427
} catch (Throwable e) {
417428
options.getLogger().log(SentryLevel.DEBUG, "Failed to send profile chunk.", e);
418429
}
419430
}
420431

421-
@VisibleForTesting
422-
@Nullable
423-
Future<?> getStopFuture() {
424-
return stopFuture;
425-
}
426-
427-
@VisibleForTesting
428-
public int getActiveTraceCount() {
429-
return activeTraceCount;
430-
}
431-
432432
/**
433433
* Collects measurements for a single profiling chunk: frame metrics (slow/frozen frames, refresh
434434
* rate) and performance data (CPU usage, memory footprint).

sentry-android-core/src/main/java/io/sentry/android/core/PerfettoProfiler.java

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -32,8 +32,12 @@ public class PerfettoProfiler {
3232
private static final String KEY_DURATION_MS = "KEY_DURATION_MS";
3333
private static final String KEY_FREQUENCY_HZ = "KEY_FREQUENCY_HZ";
3434

35-
/** Fixed sampling frequency for Perfetto stack sampling. Not configurable by the developer. */
36-
private static final int PROFILING_FREQUENCY_HZ = 100;
35+
/**
36+
* Fixed sampling frequency for Perfetto stack sampling. Not configurable by the developer. 101Hz
37+
* (rather than 100Hz) to avoid lockstep sampling with the display refresh rate (e.g. 60/120fps),
38+
* matching the legacy profiler's default sampling rate.
39+
*/
40+
private static final int PROFILING_FREQUENCY_HZ = 101;
3741

3842
private static final long RESULT_TIMEOUT_MS = 5000;
3943

sentry-android-core/src/test/java/io/sentry/android/core/PerfettoContinuousProfilerTest.kt

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,8 @@ import io.sentry.test.DeferredExecutorService
1515
import kotlin.test.AfterTest
1616
import kotlin.test.BeforeTest
1717
import kotlin.test.Test
18+
import kotlin.test.assertFalse
19+
import kotlin.test.assertNotNull
1820
import kotlin.test.assertTrue
1921
import org.junit.runner.RunWith
2022
import org.mockito.Mockito.mockStatic
@@ -170,6 +172,41 @@ class PerfettoContinuousProfilerTest {
170172

171173
// -- Perfetto-specific tests --
172174

175+
@Test
176+
fun `async chunk callback does not restart when stop requested while pending`() {
177+
val profiler = fixture.getSut()
178+
179+
// Defer the endAndCollect listener to simulate the OS delivering the trace asynchronously,
180+
// after the chunk timer already captured the (then-true) restart decision.
181+
var pendingListener: java.util.function.Consumer<java.io.File?>? = null
182+
doAnswer { invocation ->
183+
pendingListener = invocation.getArgument(0)
184+
null
185+
}
186+
.whenever(fixture.mockPerfettoProfiler)
187+
.endAndCollect(any())
188+
189+
profiler.startProfiler(ProfileLifecycle.MANUAL, fixture.mockTracesSampler)
190+
assertTrue(profiler.isRunning)
191+
192+
// Chunk timer fires: stopInternal(true) captures shouldRestart=true and calls endAndCollect,
193+
// but the listener is held pending instead of firing inline.
194+
fixture.executor.runAll()
195+
assertFalse(profiler.isRunning)
196+
assertNotNull(pendingListener)
197+
198+
// A stop is requested while the async callback is still pending.
199+
profiler.stopProfiler(ProfileLifecycle.MANUAL)
200+
201+
// The OS now delivers the trace. The callback must honor the late stop and not restart.
202+
pendingListener!!.accept(fixture.mockTraceFile)
203+
fixture.executor.runAll()
204+
assertFalse(
205+
profiler.isRunning,
206+
"profiler must not restart when a stop was requested while the callback was pending",
207+
)
208+
}
209+
173210
@Test
174211
fun `profiler multiple starts are ignored in manual mode`() {
175212
val profiler = fixture.getSut()

sentry/src/main/java/io/sentry/SentryEnvelopeItem.java

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -438,7 +438,9 @@ private static void ensureAttachmentSizeLimit(
438438
null,
439439
profileChunk.getPlatform(),
440440
null,
441-
(Callable<Integer>) metaLength::get);
441+
// avoid method refs on Android due to some issues with older AGP setups
442+
// noinspection Convert2MethodRef
443+
() -> metaLength.get());
442444

443445
// avoid method refs on Android due to some issues with older AGP setups
444446
// noinspection Convert2MethodRef

0 commit comments

Comments
 (0)