perf: Improve locks in SentryScope #888
Conversation
SentryScope contained many locks to self with @synchronized, which are not needed because not the whole scope is modified in them. Now we only lock the objects, that are modified in the @synchronized block. Fixes GH-880
Codecov Report
@@ Coverage Diff @@
## master #888 +/- ##
==========================================
+ Coverage 94.39% 94.58% +0.19%
==========================================
Files 75 75
Lines 3405 3437 +32
==========================================
+ Hits 3214 3251 +37
+ Misses 191 186 -5
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Looks much safer now. Great!
I left a few smaller notes.
Would be nice to benchmark a bit with some "real life" size scope and frequency change/clone/serialize just to understand the impact the copies will bring, and we should see some benefits of the fine grained locks.
Also it's great we're one step closer to change the scope notification for a single property and avoid reserializing the whole scope on each change.
Let's hop on a call and discuss this before merging as we chatted before?
Remove added not needed locks with #888 in SentryUser. The copy, isEqual, serialize, and hash methods don't need to be thread safe. When modifying the user from multiple threads and calling any of these methods, the callee should use locks.
Remove added not needed locks with #888 in SentryUser. The copy, isEqual, serialize, and hash methods don't need to be thread safe. When modifying the user from multiple threads and calling any of these methods, the callee should use locks.
📜 Description
SentryScope contained many locks to self with @synchronized, which are not needed because not the
whole scope is modified in them. Now we only lock the objects, that are modified in the
@synchronized block.
💡 Motivation and Context
Fixes GH-880
💚 How did you test it?
CI.
📝 Checklist
🔮 Next steps