Use synchronized instead of reentrant lock in explicit bucket histogram #6309
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #6309 +/- ##
============================================
- Coverage 91.04% 91.02% -0.02%
+ Complexity 5726 5725 -1
============================================
Files 625 625
Lines 16744 16743 -1
Branches 1713 1713
============================================
- Hits 15244 15240 -4
- Misses 1006 1008 +2
- Partials 494 495 +1 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Unfortunately, even though Handle is an inner class, instances of it are available through the createHandle() public method of the containing class. As such, a misbehaving user of this class could also synchronized(handle) and potentially form a deadlock.
I think it should be both safer and accomplish the same goals if you were to create a private final Object lock and synchronize on that instead of marking the instance methods synchronized. Make sense?
|
Meh.. I understand that calling code can also synchronize on the intrinsic lock, but: 1. This is internal code. 2. Doing so should always be done with caution. We have examples of using both the intrinsic lock and a dedicated explicit lock in internal code. Preferring one or the other is splitting hairs IMO. Let's see if any other @open-telemetry/java-approvers have a preference. If so, should probably add it to best practices. |
|
I'm with Jason on this one. It seems like a simple enough change to have an internal Object to lock on, and it will prevent having to figure out what's going on if someone decides to foot-gun themselves and synchronize on our thing. |
|
Updated to use a dedicated lock, added this advice to the best practices guide |
Was doing some analysis and noticed a small amount of allocations occurring when recording to explicit bucket histogram.
Tracked it down and determine that ReentrantLock actually performs some small amount of allocations for synchronization. See the screenshot below from my profiler. Replaced it with the
synchronizedkeyword and the allocations went away. Also noticed a modest increase in performance.HistogramBenchmark
Before
After
Notice the small improvement in
gc.alloc.rate.normand the larger-than-expected improvement inns/op.