Don't lock segment from subsegment #306
Conversation
Codecov Report
@@ Coverage Diff @@
## master #306 +/- ##
============================================
+ Coverage 58.87% 58.92% +0.04%
- Complexity 1205 1206 +1
============================================
Files 131 131
Lines 5072 5066 -6
Branches 593 593
============================================
- Hits 2986 2985 -1
+ Misses 1809 1806 -3
+ Partials 277 275 -2
Continue to review full report at Codecov.
|
| this.subsegments = new ArrayList<>(); | ||
| this.subsegments = new CopyOnWriteArrayList<>(); |
There was a problem hiding this comment.
This seems like it could get very expensive for people adding lots of subsegments... I know we don't want to spend the time for the most performant solution, but this seems like a pretty substantial degradation that would impact nearly all customers for more of an edge-case deadlock. Is there any other workaround for this?
There was a problem hiding this comment.
I think for something fairly standard like 5 subsegments, this is probably faster than something like a synchronized list.
For a hundred segments it does look slow - but so does the normal ArrayList since we call remove on it which results in quadratic behavior anyways.
I think to be optimal we would need to use a concurrent hash set, but unfortunately we need to support getSubsegments().add(). It is possible to wrap a set into a list (with slow performances of get(I)) I think, it could be an idea for if there are reports of regression in the wild.
There was a problem hiding this comment.
Ok gotcha, I guess we're also still calling remove so there's going to definitely be some degradation there. Can we add a quick additional benchmark test for creating say 100 subsegments just to confirm it's not degrading performance too badly?
There was a problem hiding this comment.
I think I'd rather close out this PR without having to too big iterations. I went and switched to synchronizedList since it'll be more similar to what we have, just a separate fine-grained lock instead of the entire segment lock.
80f0781 to
5125ec3
Compare
5125ec3 to
58b37ab
Compare
This reverts commit d5ccae2.
* Revert "Don't lock segment from subsegment (#306)" This reverts commit d5ccae2. * Revert "Keep track of emitted in emitter. (#279)" This reverts commit d66d69a. * Revert "Remove subsegments lock (#278)" This reverts commit adfb395. * Revert "Lock all accesses to entities. (#250)" This reverts commit f0a3b3a. * add back errorprone annotations * fixed shouldPropagate * added back getSubsegmentsCopy * added back compareAndSetEmitted * whitespace fix
Issue #, if available: #303
Description of changes:
High mutability and circular refrences between segments and subsegments create a prime landscape for thread issues. This tries to solve #303 by removing locking on the codepaths from
DefaultStreamingStrategy- the streaming strategy is used when processing subsegments which may be on a different thread than the segment, which could then try to stream subsegments.This should avoid deadlocks, I don't know if it's possible to be totally confident without removing streaming strategy completely - alway streaming, removing the circular references between segments / subsegments completely, would give confidence they can't deadlock. Probably best to shift to OTel instead though.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.