diff --git a/CHANGELOG.md b/CHANGELOG.md index 31c600cbcf..95a6a5b5f6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -21,6 +21,7 @@ - Avoid constructing an exception per view when resolving view ids during view-hierarchy and gesture capture ([#5631](https://github.com/getsentry/sentry-java/pull/5631)) - Start the frame metrics thread lazily on first collection instead of during SDK init ([#5641](https://github.com/getsentry/sentry-java/pull/5641)) - Reduce `SentryId` and `SpanId` allocation overhead by replacing their per-instance `LazyEvaluator` (and its lock) with a lightweight lazily-generated `String`. ([#5645](https://github.com/getsentry/sentry-java/pull/5645)) +- Lazily allocate the `ReentrantLock` backing `AutoClosableReentrantLock` to avoid eager lock allocations for SDK objects that never contend during `SentryAndroid.init` ([#5643](https://github.com/getsentry/sentry-java/pull/5643)) ## 8.46.0 diff --git a/sentry/api/sentry.api b/sentry/api/sentry.api index 04c876fdbd..383ea92b11 100644 --- a/sentry/api/sentry.api +++ b/sentry/api/sentry.api @@ -7589,9 +7589,10 @@ public abstract class io/sentry/transport/TransportResult { public static fun success ()Lio/sentry/transport/TransportResult; } -public final class io/sentry/util/AutoClosableReentrantLock : java/util/concurrent/locks/ReentrantLock { +public final class io/sentry/util/AutoClosableReentrantLock : io/sentry/ISentryLifecycleToken { public fun ()V public fun acquire ()Lio/sentry/ISentryLifecycleToken; + public fun close ()V } public final class io/sentry/util/CheckInUtils { diff --git a/sentry/src/main/java/io/sentry/util/AutoClosableReentrantLock.java b/sentry/src/main/java/io/sentry/util/AutoClosableReentrantLock.java index 2a95a58b5f..cf53d860e0 100644 --- a/sentry/src/main/java/io/sentry/util/AutoClosableReentrantLock.java +++ b/sentry/src/main/java/io/sentry/util/AutoClosableReentrantLock.java @@ -1,29 +1,70 @@ package io.sentry.util; import io.sentry.ISentryLifecycleToken; +import java.util.concurrent.atomic.AtomicReferenceFieldUpdater; import java.util.concurrent.locks.ReentrantLock; +import org.jetbrains.annotations.ApiStatus; import org.jetbrains.annotations.NotNull; +import org.jetbrains.annotations.Nullable; +import org.jetbrains.annotations.TestOnly; -public final class AutoClosableReentrantLock extends ReentrantLock { +/** + * Hands out an {@link ISentryLifecycleToken} from {@link #acquire()} for use with + * try-with-resources (replacing {@code synchronized} blocks). + * + *

The underlying {@link ReentrantLock} is created lazily on the first {@link #acquire()}. Many + * SDK objects hold a lock but never contend on it (especially during {@code SentryAndroid.init}), + * so the eager allocation of a {@link ReentrantLock} (and its {@code AbstractQueuedSynchronizer}) + * was pure GC and main-thread overhead. We keep a {@link ReentrantLock} rather than reverting to + * {@code synchronized} to stay friendly to virtual threads (Loom), see #3715. + * + *

{@link #acquire()} returns this instance as the token, so the steady-state acquire/close path + * allocates nothing. Reentrant acquires stay balanced because try-with-resources calls {@link + * #close()} exactly once per acquire. + */ +@ApiStatus.Internal +public final class AutoClosableReentrantLock implements ISentryLifecycleToken { - private static final long serialVersionUID = -3283069816958445549L; + private static final @NotNull AtomicReferenceFieldUpdater< + AutoClosableReentrantLock, ReentrantLock> + LOCK_UPDATER = + AtomicReferenceFieldUpdater.newUpdater( + AutoClosableReentrantLock.class, ReentrantLock.class, "lock"); - public ISentryLifecycleToken acquire() { - lock(); - return new AutoClosableReentrantLockLifecycleToken(this); - } + private volatile @Nullable ReentrantLock lock; - static final class AutoClosableReentrantLockLifecycleToken implements ISentryLifecycleToken { + public @NotNull ISentryLifecycleToken acquire() { + getOrCreateLock().lock(); + return this; + } - private final @NotNull ReentrantLock lock; + @Override + public void close() { + Objects.requireNonNull(lock, "close() called before acquire()").unlock(); + } - AutoClosableReentrantLockLifecycleToken(final @NotNull ReentrantLock lock) { - this.lock = lock; + private @NotNull ReentrantLock getOrCreateLock() { + final @Nullable ReentrantLock existing = lock; + if (existing != null) { + return existing; } - - @Override - public void close() { - lock.unlock(); + final @NotNull ReentrantLock candidate = new ReentrantLock(); + if (LOCK_UPDATER.compareAndSet(this, null, candidate)) { + return candidate; } + // The CAS can only fail because another thread installed its lock first, and the field is + // never reset, so all callers end up contending on that same instance. + return Objects.requireNonNull(lock, "lock must have been set by the winning thread"); + } + + @TestOnly + boolean isLocked() { + final @Nullable ReentrantLock current = lock; + return current != null && current.isLocked(); + } + + @TestOnly + boolean isLockAllocated() { + return lock != null; } } diff --git a/sentry/src/test/java/io/sentry/util/AutoClosableReentrantLockTest.kt b/sentry/src/test/java/io/sentry/util/AutoClosableReentrantLockTest.kt index 4a69b9638e..943a2c2bf7 100644 --- a/sentry/src/test/java/io/sentry/util/AutoClosableReentrantLockTest.kt +++ b/sentry/src/test/java/io/sentry/util/AutoClosableReentrantLockTest.kt @@ -1,7 +1,12 @@ package io.sentry.util +import java.util.concurrent.CountDownLatch +import java.util.concurrent.TimeUnit +import java.util.concurrent.atomic.AtomicInteger import kotlin.test.Test +import kotlin.test.assertEquals import kotlin.test.assertFalse +import kotlin.test.assertSame import kotlin.test.assertTrue class AutoClosableReentrantLockTest { @@ -11,4 +16,57 @@ class AutoClosableReentrantLockTest { lock.acquire().use { assertTrue(lock.isLocked) } assertFalse(lock.isLocked) } + + @Test + fun `acquire returns the lock itself as the token, allocating nothing`() { + val lock = AutoClosableReentrantLock() + lock.acquire().use { token -> assertSame(lock, token) } + } + + @Test + fun `does not allocate the underlying lock until first acquire`() { + val lock = AutoClosableReentrantLock() + assertFalse(lock.isLockAllocated) + lock.acquire().use {} + assertTrue(lock.isLockAllocated) + } + + @Test + fun `supports reentrant acquire from the same thread`() { + val lock = AutoClosableReentrantLock() + lock.acquire().use { + lock.acquire().use { assertTrue(lock.isLocked) } + assertTrue(lock.isLocked) + } + assertFalse(lock.isLocked) + } + + @Test + fun `mutually excludes concurrent threads`() { + val lock = AutoClosableReentrantLock() + val inCriticalSection = AtomicInteger(0) + val maxObserved = AtomicInteger(0) + val start = CountDownLatch(1) + val threadCount = 8 + val iterations = 1000 + val threads = + (0 until threadCount).map { + Thread { + start.await() + repeat(iterations) { + lock.acquire().use { + val current = inCriticalSection.incrementAndGet() + maxObserved.accumulateAndGet(current, ::maxOf) + inCriticalSection.decrementAndGet() + } + } + } + } + threads.forEach(Thread::start) + start.countDown() + threads.forEach { it.join(TimeUnit.SECONDS.toMillis(10)) } + + assertEquals(1, maxObserved.get()) + assertFalse(lock.isLocked) + } }