Skip to content

Avoid using ConcurrentLinkedDeque.size #1038

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Oct 10, 2019
Merged

Avoid using ConcurrentLinkedDeque.size #1038

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions dd-trace-ot/src/main/java/datadog/opentracing/PendingTrace.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,9 @@ public class PendingTrace extends ConcurrentLinkedDeque<DDSpan> {
Collections.newSetFromMap(new ConcurrentHashMap<WeakReference<?>, Boolean>());

private final AtomicInteger pendingReferenceCount = new AtomicInteger(0);

// We must maintain a separate count because ConcurrentLinkedDeque.size() is a linear operation.
private final AtomicInteger completedSpanCount = new AtomicInteger(0);
/**
* During a trace there are cases where the root span must be accessed (e.g. priority sampling and
* trace-search tags).
Expand Down Expand Up @@ -209,6 +212,7 @@ private void expireReference() {
final DDSpan span = it.next();
if (span != rootSpan) {
partialTrace.add(span);
completedSpanCount.decrementAndGet();
it.remove();
}
}
Expand Down Expand Up @@ -254,6 +258,17 @@ public synchronized boolean clean() {
return count > 0;
}

@Override
public void addFirst(final DDSpan span) {
super.addFirst(span);
completedSpanCount.incrementAndGet();
}

@Override
public int size() {
return completedSpanCount.get();
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 addFirst is 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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


private void addPendingTrace() {
final SpanCleaner cleaner = SPAN_CLEANER.get();
if (cleaner != null) {
Expand Down