Replace `synchronized` with`ReentrantLock` by adinauer · Pull Request #3715 · getsentry/sentry-java

adinauer · 2024-09-25T03:49:17Z

📜 Description

Most usages of synchronized keyword in method signatures and synchronized (...) { blocks have been replaced by using ReentrantLock
Adds AutoClosableReentrantLock which offers an acquire method that locks and returns an AutoClosable that unlocks on close. This can be used in try blocks
SynchronizedCollection and SynchronizedQueue now expect a AutoClosableReentrantLock instead of Object when passing a lock object
Instance locks (previously synchronized methods and synchronized (this)) now use lock
Static locks (previously static synchronized, synchronized (...class) and synchronized (...getInstance)) now use staticLock
If there was a lock object before, the type has been changed from Object to AutoClosableReentrantLock
Changing Android only classes wasn't totally necessary but IMO it's easier to maintain if there's only one way of synchronizing that's being used

💡 Motivation and Context

Fixes #3312

💚 How did you test it?

📝 Checklist

I reviewed the submitted code.
I added tests to verify the changes.
No new PII added or SDK only sends newly added PII if sendDefaultPII is enabled.
I updated the docs if needed.
Review from the native team if needed.
No breaking change or entry added to the changelog.
No breaking change for hybrid SDKs or communicated to hybrid SDKs.

🔮 Next steps

github-actions · 2024-09-25T03:58:58Z

Performance metrics 🚀

	Plain	With Sentry	Diff
Startup time	436.66 ms	493.14 ms	56.48 ms
Size	1.70 MiB	2.35 MiB	668.82 KiB

Previous results on branch: fix/replace-synchronized-with-lock

Startup times

Revision	Plain	With Sentry	Diff
`4a331fa`	408.77 ms	477.06 ms	68.29 ms

App size

Revision	Plain	With Sentry	Diff
`4a331fa`	1.70 MiB	2.35 MiB	668.79 KiB

markushi

Nice one! 🥷

markushi · 2024-09-25T10:53:58Z

  private boolean enabled;
+  private final @NotNull AutoClosableReentrantLock lock = new AutoClosableReentrantLock();

  public ActivityBreadcrumbsIntegration(final @NotNull Application application) {


To be honest I don't know why we had all the sychronized blocks in here, it seems to stem from here. I think we can remove it in future.

Suggested change

public ActivityBreadcrumbsIntegration(final @NotNull Application application) {

// TODO check if locking is even required at all for lifecycle methods

public ActivityBreadcrumbsIntegration(final @NotNull Application application) {

markushi · 2024-09-25T10:57:18Z

+    try (final @NotNull ISentryLifecycleToken ignored = lock.acquire()) {
+      // no-op
+    }


I don't think we need the locking here

Tell me how you automated that refactoring 😅

Suggested change

try (final @NotNull ISentryLifecycleToken ignored = lock.acquire()) {

// no-op

}

// no-op

I had to take a look at each thing I refactored manually 😢 . My thinking was to make this more clear in case we replace the noop comment with some actual code in the future.

Replaced the locking code with a comment that shall serve as a reminder

markushi · 2024-09-26T07:02:21Z

  private final Collection<E> collection;
  /** The object to lock on, needed for List/SortedSet views */
-  final Object lock;
+  final AutoClosableReentrantLock lock;


could this be private as well?

No because it's used in SynchronizedQueue. The same is basically true for any non final class since you have to use the right lock instead of just relying on the synchronized keyword on methods.

…ityBreadcrumbsIntegration.java Co-authored-by: Markus Hintersteiner <markus.hintersteiner@sentry.io>

github-actions · 2024-09-30T06:27:24Z

	Fails
🚫	Please consider adding a changelog entry for the next release.

Instructions and example for changelog

Please add an entry to CHANGELOG.md to the "Unreleased" section. Make sure the entry includes this PR's number.

Example:

## Unreleased

- Replace `synchronized` with`ReentrantLock` ([#3715](https://github.com/getsentry/sentry-java/pull/3715))

If none of the above apply, you can opt out of this check by adding #skip-changelog to the PR description.

Generated by 🚫 dangerJS against 2dfe2ae

AutoClosableReentrantLock extended ReentrantLock, so every SDK object holding one allocated a ReentrantLock (and its AbstractQueuedSynchronizer) eagerly in its field initializer. A customer Perfetto trace showed ~81 such allocations on the main thread during SentryAndroid.init, many for locks that are never acquired during init. Hold the ReentrantLock internally and create it lazily on first acquire(), using an AtomicReferenceFieldUpdater CAS so creation stays atomic and Loom-friendly (no synchronized, preserving #3715). Every call site uses acquire() only, so dropping the ReentrantLock superclass touches no caller. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* perf(core): Lazily allocate AutoClosableReentrantLock (JAVA-588) AutoClosableReentrantLock extended ReentrantLock, so every SDK object holding one allocated a ReentrantLock (and its AbstractQueuedSynchronizer) eagerly in its field initializer. A customer Perfetto trace showed ~81 such allocations on the main thread during SentryAndroid.init, many for locks that are never acquired during init. Hold the ReentrantLock internally and create it lazily on first acquire(), using an AtomicReferenceFieldUpdater CAS so creation stays atomic and Loom-friendly (no synchronized, preserving #3715). Every call site uses acquire() only, so dropping the ReentrantLock superclass touches no caller. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * changelog * perf(core): Harden lazy lock init and mark AutoClosableReentrantLock internal (JAVA-588) Replace the unreachable candidate fallback after a failed CAS with an explicit non-null check, so a broken invariant fails loudly instead of handing two threads different locks. Mark the class @ApiStatus.Internal and make the lazy-allocation test assert the lock field directly. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * perf(core): Return the lock itself as the lifecycle token (JAVA-588) Every acquire() allocated a fresh lifecycle token, which is per-use garbage on every lock acquisition forever, not just at init. The token was stateless apart from its lock reference, so AutoClosableReentrantLock now implements ISentryLifecycleToken itself and acquire() returns this, making the steady-state acquire/close path allocation-free. Semantics are unchanged: try-with-resources closes once per acquire, so reentrant acquires stay balanced, and unlocking without holding the lock still throws IllegalMonitorStateException. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

replace synchronized with lock

acb5ad6

adinauer requested review from markushi, romtsn and stefanosiano as code owners September 25, 2024 03:49

markushi approved these changes Sep 26, 2024

View reviewed changes

adinauer and others added 4 commits September 26, 2024 09:55

Update sentry-android-core/src/main/java/io/sentry/android/core/Activ…

b7fc22e

…ityBreadcrumbsIntegration.java Co-authored-by: Markus Hintersteiner <markus.hintersteiner@sentry.io>

changelog

eff87ba

replace locking with comment to restore the lock in case

3ec8829

Merge branch '8.x.x' into fix/replace-synchronized-with-lock

2dfe2ae

adinauer merged commit f1b7116 into 8.x.x Sep 30, 2024

adinauer deleted the fix/replace-synchronized-with-lock branch September 30, 2024 06:27

This was referenced Jun 25, 2026

Reduce AutoClosableReentrantLock allocation churn during init #5642

Closed

perf(core): Lazily allocate AutoClosableReentrantLock (JAVA-588) #5643

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

Replace `synchronized` with`ReentrantLock`#3715

Replace `synchronized` with`ReentrantLock`#3715
adinauer merged 5 commits into
8.x.xfrom
fix/replace-synchronized-with-lock

adinauer commented Sep 25, 2024

Uh oh!

github-actions Bot commented Sep 25, 2024 •

edited

Loading

Previous results on branch: fix/replace-synchronized-with-lock

Startup times

App size

Uh oh!

markushi left a comment

Uh oh!

markushi Sep 25, 2024

Uh oh!

markushi Sep 25, 2024

Uh oh!

adinauer Sep 26, 2024

Uh oh!

adinauer Sep 30, 2024

Uh oh!

markushi Sep 26, 2024

Uh oh!

adinauer Sep 26, 2024

Uh oh!

github-actions Bot commented Sep 30, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

	public ActivityBreadcrumbsIntegration(final @NotNull Application application) {
	// TODO check if locking is even required at all for lifecycle methods
	public ActivityBreadcrumbsIntegration(final @NotNull Application application) {

Uh oh!

Uh oh!

Conversation

adinauer commented Sep 25, 2024

📜 Description

💡 Motivation and Context

💚 How did you test it?

📝 Checklist

🔮 Next steps

Uh oh!

github-actions Bot commented Sep 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Performance metrics 🚀

Previous results on branch: fix/replace-synchronized-with-lock

Startup times

App size

Uh oh!

markushi left a comment

Choose a reason for hiding this comment

Uh oh!

markushi Sep 25, 2024

Choose a reason for hiding this comment

Uh oh!

markushi Sep 25, 2024

Choose a reason for hiding this comment

Uh oh!

adinauer Sep 26, 2024

Choose a reason for hiding this comment

Uh oh!

adinauer Sep 30, 2024

Choose a reason for hiding this comment

Uh oh!

markushi Sep 26, 2024

Choose a reason for hiding this comment

Uh oh!

adinauer Sep 26, 2024

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Sep 30, 2024

Instructions and example for changelog

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

github-actions Bot commented Sep 25, 2024 •

edited

Loading