Avoid using ConcurrentLinkedDeque.size #1038
Conversation
It is a linear operation that can take a long time when many spans are involved. Using an atomic integer to track the size allows it to be a constant operation.
| @Override | ||
| public int size() { | ||
| return completedSpanCount.get(); | ||
| } |
There was a problem hiding this comment.
This seems super-hackish. This class still exposes full ConcurrentLinkedDeque API but with this change it would be somewhat broken because this counter is not completed. Also addFirst is not threadsafe.
I would suggest dropping inheritance from ConcurrentLinkedDeque to remove potential confusion.
There was a problem hiding this comment.
I realize it would be better to drop the inheritance, but this is a class I see us removing in the near future, so I didn't want to put the effort into improving it right now with the priority on getting a fix out asap.
Also
addFirstis not threadsafe
Do you mean that it's possible for the count to not match the actual? I realize this is the case, but considering size is always "approximate" in a concurrent collection I felt it was an acceptable tradeoff.
There was a problem hiding this comment.
Yes, I'd agree with getting away from extendinh ConcurrentLinkedDeque or any collection for that matter.
But yes, the result of size even if accurate is potentially immediately out of date with any concurrent collection, so I think this works fine. If we don't want to change size itself, we could make a separate estimateSize routine and call that instead, but I don't feel strongly either way.
It is a linear operation that can take a long time when many spans are involved.
Using an atomic integer to track the size allows it to be a constant operation.