From 8276f08474793429c223d29626591eefc96e3b1a Mon Sep 17 00:00:00 2001 From: Alexander Dinauer Date: Wed, 30 Oct 2024 09:27:16 +0100 Subject: [PATCH 1/3] Use Random as a ThreadLocal<> --- .../android/core/AnrV2EventProcessor.java | 3 +- sentry/api/sentry.api | 5 +++ .../src/main/java/io/sentry/SentryClient.java | 5 +-- .../main/java/io/sentry/TracesSampler.java | 16 +++++-- .../java/io/sentry/util/SentryRandom.java | 20 +++++++++ .../java/io/sentry/util/SentryRandomTest.kt | 45 +++++++++++++++++++ 6 files changed, 86 insertions(+), 8 deletions(-) create mode 100644 sentry/src/main/java/io/sentry/util/SentryRandom.java create mode 100644 sentry/src/test/java/io/sentry/util/SentryRandomTest.kt diff --git a/sentry-android-core/src/main/java/io/sentry/android/core/AnrV2EventProcessor.java b/sentry-android-core/src/main/java/io/sentry/android/core/AnrV2EventProcessor.java index 0399b634fb..21d8cf8a35 100644 --- a/sentry-android-core/src/main/java/io/sentry/android/core/AnrV2EventProcessor.java +++ b/sentry-android-core/src/main/java/io/sentry/android/core/AnrV2EventProcessor.java @@ -55,6 +55,7 @@ import io.sentry.protocol.User; import io.sentry.util.HintUtils; import io.sentry.util.Random; +import io.sentry.util.SentryRandom; import java.io.File; import java.util.ArrayList; import java.util.Arrays; @@ -180,7 +181,7 @@ private boolean sampleReplay(final @NotNull SentryEvent event) { try { // we have to sample here with the old sample rate, because it may change between app launches - final @NotNull Random random = this.random != null ? this.random : new Random(); + final @NotNull Random random = this.random != null ? this.random : SentryRandom.current(); final double replayErrorSampleRateDouble = Double.parseDouble(replayErrorSampleRate); if (replayErrorSampleRateDouble < random.nextDouble()) { options diff --git a/sentry/api/sentry.api b/sentry/api/sentry.api index ed06b29b35..8e266bcdba 100644 --- a/sentry/api/sentry.api +++ b/sentry/api/sentry.api @@ -5829,6 +5829,11 @@ public final class io/sentry/util/SampleRateUtils { public static fun isValidTracesSampleRate (Ljava/lang/Double;Z)Z } +public final class io/sentry/util/SentryRandom { + public fun ()V + public static fun current ()Lio/sentry/util/Random; +} + public final class io/sentry/util/StringUtils { public static fun byteCountToString (J)Ljava/lang/String; public static fun calculateStringHash (Ljava/lang/String;Lio/sentry/ILogger;)Ljava/lang/String; diff --git a/sentry/src/main/java/io/sentry/SentryClient.java b/sentry/src/main/java/io/sentry/SentryClient.java index 27529d100b..b053230ce5 100644 --- a/sentry/src/main/java/io/sentry/SentryClient.java +++ b/sentry/src/main/java/io/sentry/SentryClient.java @@ -18,6 +18,7 @@ import io.sentry.util.HintUtils; import io.sentry.util.Objects; import io.sentry.util.Random; +import io.sentry.util.SentryRandom; import io.sentry.util.TracingUtils; import java.io.Closeable; import java.io.IOException; @@ -40,7 +41,6 @@ public final class SentryClient implements ISentryClient, IMetricsClient { private final @NotNull SentryOptions options; private final @NotNull ITransport transport; - private final @Nullable Random random; private final @NotNull SortBreadcrumbsByDate sortBreadcrumbsByDate = new SortBreadcrumbsByDate(); private final @NotNull IMetricsAggregator metricsAggregator; @@ -66,8 +66,6 @@ public boolean isEnabled() { options.isEnableMetrics() ? new MetricsAggregator(options, this) : NoopMetricsAggregator.getInstance(); - - this.random = options.getSampleRate() == null ? null : new Random(); } private boolean shouldApplyScopeData( @@ -1183,6 +1181,7 @@ public boolean isHealthy() { } private boolean sample() { + final @Nullable Random random = options.getSampleRate() == null ? null : SentryRandom.current(); // https://docs.sentry.io/development/sdk-dev/features/#event-sampling if (options.getSampleRate() != null && random != null) { final double sampling = options.getSampleRate(); diff --git a/sentry/src/main/java/io/sentry/TracesSampler.java b/sentry/src/main/java/io/sentry/TracesSampler.java index 5e5b808333..ef04cae369 100644 --- a/sentry/src/main/java/io/sentry/TracesSampler.java +++ b/sentry/src/main/java/io/sentry/TracesSampler.java @@ -2,6 +2,7 @@ import io.sentry.util.Objects; import io.sentry.util.Random; +import io.sentry.util.SentryRandom; import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; import org.jetbrains.annotations.TestOnly; @@ -10,14 +11,14 @@ final class TracesSampler { private static final @NotNull Double DEFAULT_TRACES_SAMPLE_RATE = 1.0; private final @NotNull SentryOptions options; - private final @NotNull Random random; + private final @Nullable Random random; public TracesSampler(final @NotNull SentryOptions options) { - this(Objects.requireNonNull(options, "options are required"), new Random()); + this(Objects.requireNonNull(options, "options are required"), null); } @TestOnly - TracesSampler(final @NotNull SentryOptions options, final @NotNull Random random) { + TracesSampler(final @NotNull SentryOptions options, final @Nullable Random random) { this.options = options; this.random = random; } @@ -90,6 +91,13 @@ TracesSamplingDecision sample(final @NotNull SamplingContext samplingContext) { } private boolean sample(final @NotNull Double aDouble) { - return !(aDouble < random.nextDouble()); + return !(aDouble < getRandom().nextDouble()); + } + + private Random getRandom() { + if (random == null) { + return SentryRandom.current(); + } + return random; } } diff --git a/sentry/src/main/java/io/sentry/util/SentryRandom.java b/sentry/src/main/java/io/sentry/util/SentryRandom.java new file mode 100644 index 0000000000..c266de0eee --- /dev/null +++ b/sentry/src/main/java/io/sentry/util/SentryRandom.java @@ -0,0 +1,20 @@ +package io.sentry.util; + +import org.jetbrains.annotations.NotNull; + +public final class SentryRandom { + + private static final SentryRandomThreadLocal instance = new SentryRandomThreadLocal(); + + public static @NotNull Random current() { + return instance.get(); + } + + private static class SentryRandomThreadLocal extends ThreadLocal { + + @Override + protected Random initialValue() { + return new Random(); + } + } +} diff --git a/sentry/src/test/java/io/sentry/util/SentryRandomTest.kt b/sentry/src/test/java/io/sentry/util/SentryRandomTest.kt new file mode 100644 index 0000000000..a9c52d7cbf --- /dev/null +++ b/sentry/src/test/java/io/sentry/util/SentryRandomTest.kt @@ -0,0 +1,45 @@ +package io.sentry.util + +import kotlin.test.Test +import kotlin.test.assertEquals +import kotlin.test.assertNotEquals + +class SentryRandomTest { + + @Test + fun `thread local creates a new instance per thread but keeps re-using it for the same thread`() { + val mainThreadRandom1 = SentryRandom.current() + val mainThreadRandom2 = SentryRandom.current() + assertEquals(mainThreadRandom1.toString(), mainThreadRandom2.toString()) + + var thread1Random1: Random? = null + var thread1Random2: Random? = null + + val thread1 = Thread() { + thread1Random1 = SentryRandom.current() + thread1Random2 = SentryRandom.current() + } + + var thread2Random1: Random? = null + var thread2Random2: Random? = null + + val thread2 = Thread() { + thread2Random1 = SentryRandom.current() + thread2Random2 = SentryRandom.current() + } + + thread1.start() + thread2.start() + thread1.join() + thread2.join() + + assertEquals(thread1Random1.toString(), thread1Random2.toString()) + assertNotEquals(mainThreadRandom1.toString(), thread1Random1.toString()) + + assertEquals(thread2Random1.toString(), thread2Random2.toString()) + assertNotEquals(mainThreadRandom1.toString(), thread2Random1.toString()) + + val mainThreadRandom3 = SentryRandom.current() + assertEquals(mainThreadRandom1.toString(), mainThreadRandom3.toString()) + } +} From a6afc133e536f6bcc9ad61861b5bbbbc9815fb72 Mon Sep 17 00:00:00 2001 From: Alexander Dinauer Date: Thu, 31 Oct 2024 06:27:36 +0100 Subject: [PATCH 2/3] changelog --- CHANGELOG.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 813e9e65bc..cd68c3158e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,10 @@ ## Unreleased +### Features + +- Use a separate `Random` instance per thread to improve SDK performance ([#3835](https://github.com/getsentry/sentry-java/pull/3835)) + ### Fixes - Accept manifest integer values when requiring floating values ([#3823](https://github.com/getsentry/sentry-java/pull/3823)) From 5863675e83d1c8dbc96dfba0ec5e5b741ba551bb Mon Sep 17 00:00:00 2001 From: Alexander Dinauer Date: Thu, 31 Oct 2024 06:28:04 +0100 Subject: [PATCH 3/3] code review changes --- .../android/core/AnrV2EventProcessor.java | 15 +-------------- .../java/io/sentry/util/SentryRandom.java | 19 ++++++++++++++++++- .../java/io/sentry/util/SentryRandomTest.kt | 16 ++++++++-------- 3 files changed, 27 insertions(+), 23 deletions(-) diff --git a/sentry-android-core/src/main/java/io/sentry/android/core/AnrV2EventProcessor.java b/sentry-android-core/src/main/java/io/sentry/android/core/AnrV2EventProcessor.java index 21d8cf8a35..e914029c30 100644 --- a/sentry-android-core/src/main/java/io/sentry/android/core/AnrV2EventProcessor.java +++ b/sentry-android-core/src/main/java/io/sentry/android/core/AnrV2EventProcessor.java @@ -54,7 +54,6 @@ import io.sentry.protocol.SentryTransaction; import io.sentry.protocol.User; import io.sentry.util.HintUtils; -import io.sentry.util.Random; import io.sentry.util.SentryRandom; import java.io.File; import java.util.ArrayList; @@ -84,24 +83,13 @@ public final class AnrV2EventProcessor implements BackfillingEventProcessor { private final @NotNull SentryExceptionFactory sentryExceptionFactory; - private final @Nullable Random random; - public AnrV2EventProcessor( final @NotNull Context context, final @NotNull SentryAndroidOptions options, final @NotNull BuildInfoProvider buildInfoProvider) { - this(context, options, buildInfoProvider, null); - } - - AnrV2EventProcessor( - final @NotNull Context context, - final @NotNull SentryAndroidOptions options, - final @NotNull BuildInfoProvider buildInfoProvider, - final @Nullable Random random) { this.context = ContextUtils.getApplicationContext(context); this.options = options; this.buildInfoProvider = buildInfoProvider; - this.random = random; final SentryStackTraceFactory sentryStackTraceFactory = new SentryStackTraceFactory(this.options); @@ -181,9 +169,8 @@ private boolean sampleReplay(final @NotNull SentryEvent event) { try { // we have to sample here with the old sample rate, because it may change between app launches - final @NotNull Random random = this.random != null ? this.random : SentryRandom.current(); final double replayErrorSampleRateDouble = Double.parseDouble(replayErrorSampleRate); - if (replayErrorSampleRateDouble < random.nextDouble()) { + if (replayErrorSampleRateDouble < SentryRandom.current().nextDouble()) { options .getLogger() .log( diff --git a/sentry/src/main/java/io/sentry/util/SentryRandom.java b/sentry/src/main/java/io/sentry/util/SentryRandom.java index c266de0eee..f6e1d0a974 100644 --- a/sentry/src/main/java/io/sentry/util/SentryRandom.java +++ b/sentry/src/main/java/io/sentry/util/SentryRandom.java @@ -2,10 +2,27 @@ import org.jetbrains.annotations.NotNull; +/** + * This SentryRandom is a compromise used for improving performance of the SDK. + * + *

We did some testing where using Random from multiple threads degrades performance + * significantly. We opted for this approach as it wasn't easily possible to vendor + * ThreadLocalRandom since it's using advanced features that can cause java.lang.IllegalAccessError. + */ public final class SentryRandom { - private static final SentryRandomThreadLocal instance = new SentryRandomThreadLocal(); + private static final @NotNull SentryRandomThreadLocal instance = new SentryRandomThreadLocal(); + /** + * Returns the current threads instance of {@link Random}. An instance of {@link Random} will be + * created the first time this is invoked on each thread. + * + *

NOTE: Avoid holding a reference to the returned {@link Random} instance as sharing a + * reference across threads (while being thread-safe) will likely degrade performance + * significantly. + * + * @return random + */ public static @NotNull Random current() { return instance.get(); } diff --git a/sentry/src/test/java/io/sentry/util/SentryRandomTest.kt b/sentry/src/test/java/io/sentry/util/SentryRandomTest.kt index a9c52d7cbf..c812c6cbdc 100644 --- a/sentry/src/test/java/io/sentry/util/SentryRandomTest.kt +++ b/sentry/src/test/java/io/sentry/util/SentryRandomTest.kt @@ -1,8 +1,8 @@ package io.sentry.util import kotlin.test.Test -import kotlin.test.assertEquals -import kotlin.test.assertNotEquals +import kotlin.test.assertNotSame +import kotlin.test.assertSame class SentryRandomTest { @@ -10,7 +10,7 @@ class SentryRandomTest { fun `thread local creates a new instance per thread but keeps re-using it for the same thread`() { val mainThreadRandom1 = SentryRandom.current() val mainThreadRandom2 = SentryRandom.current() - assertEquals(mainThreadRandom1.toString(), mainThreadRandom2.toString()) + assertSame(mainThreadRandom1, mainThreadRandom2) var thread1Random1: Random? = null var thread1Random2: Random? = null @@ -33,13 +33,13 @@ class SentryRandomTest { thread1.join() thread2.join() - assertEquals(thread1Random1.toString(), thread1Random2.toString()) - assertNotEquals(mainThreadRandom1.toString(), thread1Random1.toString()) + assertSame(thread1Random1, thread1Random2) + assertNotSame(mainThreadRandom1, thread1Random1) - assertEquals(thread2Random1.toString(), thread2Random2.toString()) - assertNotEquals(mainThreadRandom1.toString(), thread2Random1.toString()) + assertSame(thread2Random1, thread2Random2) + assertNotSame(mainThreadRandom1, thread2Random1) val mainThreadRandom3 = SentryRandom.current() - assertEquals(mainThreadRandom1.toString(), mainThreadRandom3.toString()) + assertSame(mainThreadRandom1, mainThreadRandom3) } }