From 91bb628e08eecbd1fce793794d6e8e2095f119bb Mon Sep 17 00:00:00 2001 From: Roman Zavarnitsyn Date: Fri, 20 Dec 2024 18:24:31 +0100 Subject: [PATCH] [SR] Fix ANRs and speed up (#4001) * Get rid of the lock on touch events * pre-allocate some things for gesture converter * have one less thread switch for re]play * update * Changelog --- CHANGELOG.md | 1 + .../android/replay/ReplayIntegration.kt | 26 +++++++++--- .../android/replay/ScreenshotRecorder.kt | 10 ++--- .../sentry/android/replay/WindowRecorder.kt | 8 +++- .../replay/capture/BaseCaptureStrategy.kt | 42 ++++--------------- .../replay/capture/BufferCaptureStrategy.kt | 6 ++- .../android/replay/capture/CaptureStrategy.kt | 20 ++++----- .../replay/capture/SessionCaptureStrategy.kt | 2 +- .../replay/gestures/ReplayGestureConverter.kt | 4 +- .../sentry/android/replay/util/Executors.kt | 5 +++ .../sentry/android/replay/util/Persistable.kt | 8 +++- .../android/replay/ReplayIntegrationTest.kt | 1 - .../ReplayIntegrationWithRecorderTest.kt | 6 +-- 13 files changed, 69 insertions(+), 70 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2d1a18877f..3dffe87b4c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,7 @@ - Change TTFD timeout to 25 seconds ([#3984](https://github.com/getsentry/sentry-java/pull/3984)) - Session Replay: Fix memory leak when masking Compose screens ([#3985](https://github.com/getsentry/sentry-java/pull/3985)) +- Session Replay: Fix potential ANRs in `GestureRecorder` ([#4001](https://github.com/getsentry/sentry-java/pull/4001)) ## 7.19.0 diff --git a/sentry-android-replay/src/main/java/io/sentry/android/replay/ReplayIntegration.kt b/sentry-android-replay/src/main/java/io/sentry/android/replay/ReplayIntegration.kt index 66bd17b743..5b7e3ecae6 100644 --- a/sentry-android-replay/src/main/java/io/sentry/android/replay/ReplayIntegration.kt +++ b/sentry-android-replay/src/main/java/io/sentry/android/replay/ReplayIntegration.kt @@ -29,6 +29,7 @@ import io.sentry.android.replay.gestures.GestureRecorder import io.sentry.android.replay.gestures.TouchRecorderCallback import io.sentry.android.replay.util.MainLooperHandler import io.sentry.android.replay.util.appContext +import io.sentry.android.replay.util.gracefullyShutdown import io.sentry.android.replay.util.sample import io.sentry.android.replay.util.submitSafely import io.sentry.cache.PersistingScopeObserver @@ -46,8 +47,9 @@ import io.sentry.util.Random import java.io.Closeable import java.io.File import java.util.LinkedList +import java.util.concurrent.Executors +import java.util.concurrent.ThreadFactory import java.util.concurrent.atomic.AtomicBoolean -import kotlin.LazyThreadSafetyMode.NONE public class ReplayIntegration( private val context: Context, @@ -93,7 +95,10 @@ public class ReplayIntegration( private var recorder: Recorder? = null private var gestureRecorder: GestureRecorder? = null private val random by lazy { Random() } - internal val rootViewsSpy by lazy(NONE) { RootViewsSpy.install() } + internal val rootViewsSpy by lazy { RootViewsSpy.install() } + private val replayExecutor by lazy { + Executors.newSingleThreadScheduledExecutor(ReplayExecutorServiceThreadFactory()) + } // TODO: probably not everything has to be thread-safe here internal val isEnabled = AtomicBoolean(false) @@ -123,7 +128,7 @@ public class ReplayIntegration( } this.hub = hub - recorder = recorderProvider?.invoke() ?: WindowRecorder(options, this, mainLooperHandler) + recorder = recorderProvider?.invoke() ?: WindowRecorder(options, this, mainLooperHandler, replayExecutor) gestureRecorder = gestureRecorderProvider?.invoke() ?: GestureRecorder(options, this) isEnabled.set(true) @@ -166,9 +171,9 @@ public class ReplayIntegration( recorderConfig = recorderConfigProvider?.invoke(false) ?: ScreenshotRecorderConfig.from(context, options.experimental.sessionReplay) captureStrategy = replayCaptureStrategyProvider?.invoke(isFullSession) ?: if (isFullSession) { - SessionCaptureStrategy(options, hub, dateProvider, replayCacheProvider = replayCacheProvider) + SessionCaptureStrategy(options, hub, dateProvider, replayExecutor, replayCacheProvider) } else { - BufferCaptureStrategy(options, hub, dateProvider, random, replayCacheProvider = replayCacheProvider) + BufferCaptureStrategy(options, hub, dateProvider, random, replayExecutor, replayCacheProvider) } captureStrategy?.start(recorderConfig) @@ -229,7 +234,6 @@ public class ReplayIntegration( gestureRecorder?.stop() captureStrategy?.stop() isRecording.set(false) - captureStrategy?.close() captureStrategy = null } @@ -264,6 +268,7 @@ public class ReplayIntegration( recorder?.close() recorder = null rootViewsSpy.close() + replayExecutor.gracefullyShutdown(options) } override fun onConfigurationChanged(newConfig: Configuration) { @@ -405,4 +410,13 @@ public class ReplayIntegration( private class PreviousReplayHint : Backfillable { override fun shouldEnrich(): Boolean = false } + + private class ReplayExecutorServiceThreadFactory : ThreadFactory { + private var cnt = 0 + override fun newThread(r: Runnable): Thread { + val ret = Thread(r, "SentryReplayIntegration-" + cnt++) + ret.setDaemon(true) + return ret + } + } } diff --git a/sentry-android-replay/src/main/java/io/sentry/android/replay/ScreenshotRecorder.kt b/sentry-android-replay/src/main/java/io/sentry/android/replay/ScreenshotRecorder.kt index 8ca9fed7fe..6f588fe779 100644 --- a/sentry-android-replay/src/main/java/io/sentry/android/replay/ScreenshotRecorder.kt +++ b/sentry-android-replay/src/main/java/io/sentry/android/replay/ScreenshotRecorder.kt @@ -25,7 +25,6 @@ import io.sentry.SentryReplayOptions import io.sentry.android.replay.util.MainLooperHandler import io.sentry.android.replay.util.addOnDrawListenerSafe import io.sentry.android.replay.util.getVisibleRects -import io.sentry.android.replay.util.gracefullyShutdown import io.sentry.android.replay.util.removeOnDrawListenerSafe import io.sentry.android.replay.util.submitSafely import io.sentry.android.replay.util.traverse @@ -34,7 +33,7 @@ import io.sentry.android.replay.viewhierarchy.ViewHierarchyNode.ImageViewHierarc import io.sentry.android.replay.viewhierarchy.ViewHierarchyNode.TextViewHierarchyNode import java.io.File import java.lang.ref.WeakReference -import java.util.concurrent.Executors +import java.util.concurrent.ScheduledExecutorService import java.util.concurrent.ThreadFactory import java.util.concurrent.atomic.AtomicBoolean import kotlin.LazyThreadSafetyMode.NONE @@ -44,13 +43,11 @@ import kotlin.math.roundToInt internal class ScreenshotRecorder( val config: ScreenshotRecorderConfig, val options: SentryOptions, - val mainLooperHandler: MainLooperHandler, + private val mainLooperHandler: MainLooperHandler, + private val recorder: ScheduledExecutorService, private val screenshotRecorderCallback: ScreenshotRecorderCallback? ) : ViewTreeObserver.OnDrawListener { - private val recorder by lazy { - Executors.newSingleThreadScheduledExecutor(RecorderExecutorServiceThreadFactory()) - } private var rootView: WeakReference? = null private val maskingPaint by lazy(NONE) { Paint() } private val singlePixelBitmap: Bitmap by lazy(NONE) { @@ -231,7 +228,6 @@ internal class ScreenshotRecorder( rootView?.clear() lastScreenshot?.recycle() isCapturing.set(false) - recorder.gracefullyShutdown(options) } private fun Bitmap.dominantColorForRect(rect: Rect): Int { diff --git a/sentry-android-replay/src/main/java/io/sentry/android/replay/WindowRecorder.kt b/sentry-android-replay/src/main/java/io/sentry/android/replay/WindowRecorder.kt index 35b5690a8d..8f4b0526fc 100644 --- a/sentry-android-replay/src/main/java/io/sentry/android/replay/WindowRecorder.kt +++ b/sentry-android-replay/src/main/java/io/sentry/android/replay/WindowRecorder.kt @@ -8,6 +8,7 @@ import io.sentry.android.replay.util.gracefullyShutdown import io.sentry.android.replay.util.scheduleAtFixedRateSafely import java.lang.ref.WeakReference import java.util.concurrent.Executors +import java.util.concurrent.ScheduledExecutorService import java.util.concurrent.ScheduledFuture import java.util.concurrent.ThreadFactory import java.util.concurrent.TimeUnit.MILLISECONDS @@ -17,7 +18,8 @@ import java.util.concurrent.atomic.AtomicBoolean internal class WindowRecorder( private val options: SentryOptions, private val screenshotRecorderCallback: ScreenshotRecorderCallback? = null, - private val mainLooperHandler: MainLooperHandler + private val mainLooperHandler: MainLooperHandler, + private val replayExecutor: ScheduledExecutorService ) : Recorder, OnRootViewsChangedListener { internal companion object { @@ -57,7 +59,9 @@ internal class WindowRecorder( return } - recorder = ScreenshotRecorder(recorderConfig, options, mainLooperHandler, screenshotRecorderCallback) + recorder = ScreenshotRecorder(recorderConfig, options, mainLooperHandler, replayExecutor, screenshotRecorderCallback) + // TODO: change this to use MainThreadHandler and just post on the main thread with delay + // to avoid thread context switch every time capturingTask = capturer.scheduleAtFixedRateSafely( options, "$TAG.capture", diff --git a/sentry-android-replay/src/main/java/io/sentry/android/replay/capture/BaseCaptureStrategy.kt b/sentry-android-replay/src/main/java/io/sentry/android/replay/capture/BaseCaptureStrategy.kt index 4b37eafc78..df0b7575a3 100644 --- a/sentry-android-replay/src/main/java/io/sentry/android/replay/capture/BaseCaptureStrategy.kt +++ b/sentry-android-replay/src/main/java/io/sentry/android/replay/capture/BaseCaptureStrategy.kt @@ -1,5 +1,6 @@ package io.sentry.android.replay.capture +import android.annotation.TargetApi import android.view.MotionEvent import io.sentry.Breadcrumb import io.sentry.DateUtils @@ -14,25 +15,22 @@ import io.sentry.android.replay.ReplayCache.Companion.SEGMENT_KEY_FRAME_RATE import io.sentry.android.replay.ReplayCache.Companion.SEGMENT_KEY_HEIGHT import io.sentry.android.replay.ReplayCache.Companion.SEGMENT_KEY_ID import io.sentry.android.replay.ReplayCache.Companion.SEGMENT_KEY_REPLAY_ID -import io.sentry.android.replay.ReplayCache.Companion.SEGMENT_KEY_REPLAY_RECORDING import io.sentry.android.replay.ReplayCache.Companion.SEGMENT_KEY_REPLAY_SCREEN_AT_START import io.sentry.android.replay.ReplayCache.Companion.SEGMENT_KEY_REPLAY_TYPE import io.sentry.android.replay.ReplayCache.Companion.SEGMENT_KEY_TIMESTAMP import io.sentry.android.replay.ReplayCache.Companion.SEGMENT_KEY_WIDTH import io.sentry.android.replay.ScreenshotRecorderConfig import io.sentry.android.replay.capture.CaptureStrategy.Companion.createSegment -import io.sentry.android.replay.capture.CaptureStrategy.Companion.currentEventsLock import io.sentry.android.replay.capture.CaptureStrategy.ReplaySegment import io.sentry.android.replay.gestures.ReplayGestureConverter -import io.sentry.android.replay.util.PersistableLinkedList -import io.sentry.android.replay.util.gracefullyShutdown import io.sentry.android.replay.util.submitSafely import io.sentry.protocol.SentryId import io.sentry.rrweb.RRWebEvent import io.sentry.transport.ICurrentDateProvider import java.io.File import java.util.Date -import java.util.LinkedList +import java.util.Deque +import java.util.concurrent.ConcurrentLinkedDeque import java.util.concurrent.Executors import java.util.concurrent.ScheduledExecutorService import java.util.concurrent.ThreadFactory @@ -42,11 +40,12 @@ import java.util.concurrent.atomic.AtomicReference import kotlin.properties.ReadWriteProperty import kotlin.reflect.KProperty +@TargetApi(26) internal abstract class BaseCaptureStrategy( private val options: SentryOptions, private val hub: IHub?, private val dateProvider: ICurrentDateProvider, - executor: ScheduledExecutorService? = null, + protected val replayExecutor: ScheduledExecutorService, private val replayCacheProvider: ((replayId: SentryId, recorderConfig: ScreenshotRecorderConfig) -> ReplayCache)? = null ) : CaptureStrategy { @@ -81,16 +80,8 @@ internal abstract class BaseCaptureStrategy( override val replayCacheDir: File? get() = cache?.replayCacheDir override var replayType by persistableAtomic(propertyName = SEGMENT_KEY_REPLAY_TYPE) - protected val currentEvents: LinkedList = PersistableLinkedList( - propertyName = SEGMENT_KEY_REPLAY_RECORDING, - options, - persistingExecutor, - cacheProvider = { cache } - ) - - protected val replayExecutor: ScheduledExecutorService by lazy { - executor ?: Executors.newSingleThreadScheduledExecutor(ReplayExecutorServiceThreadFactory()) - } + + protected val currentEvents: Deque = ConcurrentLinkedDeque() override fun start( recorderConfig: ScreenshotRecorderConfig, @@ -135,7 +126,7 @@ internal abstract class BaseCaptureStrategy( frameRate: Int = recorderConfig.frameRate, screenAtStart: String? = this.screenAtStart, breadcrumbs: List? = null, - events: LinkedList = this.currentEvents + events: Deque = this.currentEvents ): ReplaySegment = createSegment( hub, @@ -161,22 +152,7 @@ internal abstract class BaseCaptureStrategy( override fun onTouchEvent(event: MotionEvent) { val rrwebEvents = gestureConverter.convert(event, recorderConfig) if (rrwebEvents != null) { - synchronized(currentEventsLock) { - currentEvents += rrwebEvents - } - } - } - - override fun close() { - replayExecutor.gracefullyShutdown(options) - } - - private class ReplayExecutorServiceThreadFactory : ThreadFactory { - private var cnt = 0 - override fun newThread(r: Runnable): Thread { - val ret = Thread(r, "SentryReplayIntegration-" + cnt++) - ret.setDaemon(true) - return ret + currentEvents += rrwebEvents } } diff --git a/sentry-android-replay/src/main/java/io/sentry/android/replay/capture/BufferCaptureStrategy.kt b/sentry-android-replay/src/main/java/io/sentry/android/replay/capture/BufferCaptureStrategy.kt index 2dbb2a1746..0418283dda 100644 --- a/sentry-android-replay/src/main/java/io/sentry/android/replay/capture/BufferCaptureStrategy.kt +++ b/sentry-android-replay/src/main/java/io/sentry/android/replay/capture/BufferCaptureStrategy.kt @@ -1,5 +1,6 @@ package io.sentry.android.replay.capture +import android.annotation.TargetApi import android.graphics.Bitmap import android.view.MotionEvent import io.sentry.DateUtils @@ -23,14 +24,15 @@ import java.io.File import java.util.Date import java.util.concurrent.ScheduledExecutorService +@TargetApi(26) internal class BufferCaptureStrategy( private val options: SentryOptions, private val hub: IHub?, private val dateProvider: ICurrentDateProvider, private val random: Random, - executor: ScheduledExecutorService? = null, + executor: ScheduledExecutorService, replayCacheProvider: ((replayId: SentryId, recorderConfig: ScreenshotRecorderConfig) -> ReplayCache)? = null -) : BaseCaptureStrategy(options, hub, dateProvider, executor = executor, replayCacheProvider = replayCacheProvider) { +) : BaseCaptureStrategy(options, hub, dateProvider, executor, replayCacheProvider = replayCacheProvider) { // TODO: capture envelopes for buffered segments instead, but don't send them until buffer is triggered private val bufferedSegments = mutableListOf() diff --git a/sentry-android-replay/src/main/java/io/sentry/android/replay/capture/CaptureStrategy.kt b/sentry-android-replay/src/main/java/io/sentry/android/replay/capture/CaptureStrategy.kt index 53f5c66683..f4e6215710 100644 --- a/sentry-android-replay/src/main/java/io/sentry/android/replay/capture/CaptureStrategy.kt +++ b/sentry-android-replay/src/main/java/io/sentry/android/replay/capture/CaptureStrategy.kt @@ -19,6 +19,7 @@ import io.sentry.rrweb.RRWebMetaEvent import io.sentry.rrweb.RRWebVideoEvent import java.io.File import java.util.Date +import java.util.Deque import java.util.LinkedList internal interface CaptureStrategy { @@ -53,11 +54,8 @@ internal interface CaptureStrategy { fun convert(): CaptureStrategy - fun close() - companion object { private const val BREADCRUMB_START_OFFSET = 100L - internal val currentEventsLock = Any() fun createSegment( hub: IHub?, @@ -73,7 +71,7 @@ internal interface CaptureStrategy { frameRate: Int, screenAtStart: String?, breadcrumbs: List?, - events: LinkedList + events: Deque ): ReplaySegment { val generatedVideo = cache?.createVideoOf( duration, @@ -127,7 +125,7 @@ internal interface CaptureStrategy { replayType: ReplayType, screenAtStart: String?, breadcrumbs: List, - events: LinkedList + events: Deque ): ReplaySegment { val endTimestamp = DateUtils.getDateTime(segmentTimestamp.time + videoDuration) val replay = SentryReplayEvent().apply { @@ -207,16 +205,16 @@ internal interface CaptureStrategy { } internal fun rotateEvents( - events: LinkedList, + events: Deque, until: Long, callback: ((RRWebEvent) -> Unit)? = null ) { - synchronized(currentEventsLock) { - var event = events.peek() - while (event != null && event.timestamp < until) { + val iter = events.iterator() + while (iter.hasNext()) { + val event = iter.next() + if (event.timestamp < until) { callback?.invoke(event) - events.remove() - event = events.peek() + iter.remove() } } } diff --git a/sentry-android-replay/src/main/java/io/sentry/android/replay/capture/SessionCaptureStrategy.kt b/sentry-android-replay/src/main/java/io/sentry/android/replay/capture/SessionCaptureStrategy.kt index 08aec7e9f7..1a6dbc8c89 100644 --- a/sentry-android-replay/src/main/java/io/sentry/android/replay/capture/SessionCaptureStrategy.kt +++ b/sentry-android-replay/src/main/java/io/sentry/android/replay/capture/SessionCaptureStrategy.kt @@ -20,7 +20,7 @@ internal class SessionCaptureStrategy( private val options: SentryOptions, private val hub: IHub?, private val dateProvider: ICurrentDateProvider, - executor: ScheduledExecutorService? = null, + executor: ScheduledExecutorService, replayCacheProvider: ((replayId: SentryId, recorderConfig: ScreenshotRecorderConfig) -> ReplayCache)? = null ) : BaseCaptureStrategy(options, hub, dateProvider, executor, replayCacheProvider) { diff --git a/sentry-android-replay/src/main/java/io/sentry/android/replay/gestures/ReplayGestureConverter.kt b/sentry-android-replay/src/main/java/io/sentry/android/replay/gestures/ReplayGestureConverter.kt index 59d6b30bce..fa215960c4 100644 --- a/sentry-android-replay/src/main/java/io/sentry/android/replay/gestures/ReplayGestureConverter.kt +++ b/sentry-android-replay/src/main/java/io/sentry/android/replay/gestures/ReplayGestureConverter.kt @@ -56,7 +56,7 @@ class ReplayGestureConverter( val totalOffset = now - touchMoveBaseline return if (totalOffset > CAPTURE_MOVE_EVENT_THRESHOLD) { - val moveEvents = mutableListOf() + val moveEvents = ArrayList(currentPositions.size) for ((pointerId, positions) in currentPositions) { if (positions.isNotEmpty()) { moveEvents += RRWebInteractionMoveEvent().apply { @@ -88,7 +88,7 @@ class ReplayGestureConverter( } // new finger down - add a new pointer for tracking movement - currentPositions[pId] = ArrayList() + currentPositions[pId] = ArrayList(10) listOf( RRWebInteractionEvent().apply { timestamp = dateProvider.currentTimeMillis diff --git a/sentry-android-replay/src/main/java/io/sentry/android/replay/util/Executors.kt b/sentry-android-replay/src/main/java/io/sentry/android/replay/util/Executors.kt index 453ff49df2..3c07ad7eaa 100644 --- a/sentry-android-replay/src/main/java/io/sentry/android/replay/util/Executors.kt +++ b/sentry-android-replay/src/main/java/io/sentry/android/replay/util/Executors.kt @@ -50,6 +50,11 @@ internal fun ExecutorService.submitSafely( taskName: String, task: Runnable ): Future<*>? { + if (Thread.currentThread().name.startsWith("SentryReplayIntegration")) { + // we're already on the worker thread, no need to submit + task.run() + return null + } return try { submit { try { diff --git a/sentry-android-replay/src/main/java/io/sentry/android/replay/util/Persistable.kt b/sentry-android-replay/src/main/java/io/sentry/android/replay/util/Persistable.kt index 553bae8dee..2ae3bad1b6 100644 --- a/sentry-android-replay/src/main/java/io/sentry/android/replay/util/Persistable.kt +++ b/sentry-android-replay/src/main/java/io/sentry/android/replay/util/Persistable.kt @@ -1,21 +1,25 @@ // ktlint-disable filename package io.sentry.android.replay.util +import android.annotation.TargetApi import io.sentry.ReplayRecording import io.sentry.SentryOptions import io.sentry.android.replay.ReplayCache import io.sentry.rrweb.RRWebEvent import java.io.BufferedWriter import java.io.StringWriter -import java.util.LinkedList +import java.util.concurrent.ConcurrentLinkedDeque import java.util.concurrent.ScheduledExecutorService +// TODO: enable this back after we are able to serialize individual touches to disk to not overload cpu +@Suppress("unused") +@TargetApi(26) internal class PersistableLinkedList( private val propertyName: String, private val options: SentryOptions, private val persistingExecutor: ScheduledExecutorService, private val cacheProvider: () -> ReplayCache? -) : LinkedList() { +) : ConcurrentLinkedDeque() { // only overriding methods that we use, to observe the collection override fun addAll(elements: Collection): Boolean { val result = super.addAll(elements) diff --git a/sentry-android-replay/src/test/java/io/sentry/android/replay/ReplayIntegrationTest.kt b/sentry-android-replay/src/test/java/io/sentry/android/replay/ReplayIntegrationTest.kt index ff396d04c1..95380deaa7 100644 --- a/sentry-android-replay/src/test/java/io/sentry/android/replay/ReplayIntegrationTest.kt +++ b/sentry-android-replay/src/test/java/io/sentry/android/replay/ReplayIntegrationTest.kt @@ -411,7 +411,6 @@ class ReplayIntegrationTest { verify(recorder).stop() verify(recorder).close() verify(captureStrategy).stop() - verify(captureStrategy).close() assertFalse(replay.isRecording()) } diff --git a/sentry-android-replay/src/test/java/io/sentry/android/replay/ReplayIntegrationWithRecorderTest.kt b/sentry-android-replay/src/test/java/io/sentry/android/replay/ReplayIntegrationWithRecorderTest.kt index 6bab0f6549..ea28ce7e54 100644 --- a/sentry-android-replay/src/test/java/io/sentry/android/replay/ReplayIntegrationWithRecorderTest.kt +++ b/sentry-android-replay/src/test/java/io/sentry/android/replay/ReplayIntegrationWithRecorderTest.kt @@ -136,9 +136,6 @@ class ReplayIntegrationWithRecorderTest { replay.stop() assertEquals(STOPPED, recorder.state) - replay.close() - assertEquals(CLOSED, recorder.state) - // start again and capture some frames replay.start() @@ -176,6 +173,9 @@ class ReplayIntegrationWithRecorderTest { assertEquals(0, videoEvents?.first()?.segmentId) } ) + + replay.close() + assertEquals(CLOSED, recorder.state) } enum class LifecycleState {