Avoid creation of empty CopyOnWriteArrayList for span links#10822
Avoid creation of empty CopyOnWriteArrayList for span links#10822gh-worker-dd-mergequeue-cf854d[bot] merged 13 commits intomasterfrom
Conversation
This change shares an "EMPTY" list between span when there are no span links. This avoids the construction of a CopyOnWriteArrayList, empty Object[], and internal lock Object for the common case where there are no links. This elimination of the CopyOnWriteArrayList in the empty case, reduced allocation by 5% and GC time by 7% in a span creation stress test.
dougqh
added
type: enhancement
amarziali
left a comment
amarziali
left a comment
There was a problem hiding this comment.
thanks for the improvement
| private volatile int longRunningVersion = 0; | ||
|
|
||
| protected final List<AgentSpanLink> links; | ||
| private static final List<AgentSpanLink> EMPTY = Collections.emptyList(); |
There was a problem hiding this comment.
There is an issue with SpanPointersProcessor. In fact TagsPostProcessors manipulate links directly and adding something to an emptyList will cause an UnsupportedOperationException. What can be done, is to use an empty non threadsafe list when calling tag processors if the original is empty and eventually batch adding the list at the end if the processors added something
There was a problem hiding this comment.
Few ideas here:
- There is a TODO from Doug about the processor that mutates the span links:
Does it mean it can skip span links and use tags directly?
- As processors only add span links, we can replace the span links collection by a method reference to the
addSpanLinkmethod asConsumer<SpanLink>. This will avoid exposing the span link implementation and we can keep itnulluntil there is effectively span link stored.
There was a problem hiding this comment.
That's a good catch.
I do think we need to improve the encapsulation; otherwise, these changes are going to be hard to make.
As for my prior TODO, TagMap has a bunch of methods for removeAndGetEntry, getString, stringValue, etc that could be used in the various *spanPointer methods. In theory, they could simplify the code significantly.
There was a problem hiding this comment.
On a related note, I'm curious why spanLinks was put on DDSpan rather than DDSpanContext.
It seems like it would be more consistent to put on DDSpanContext and then we wouldn't have this issue.
I also think we might be pool DDSpanContext in the future, so I'd like to avoid adding more to DDSpan.
There was a problem hiding this comment.
I'm curious why spanLinks was put on DDSpan rather than DDSpanContext.
SpanContext was supposed to describe "a span context", not to hold the span data model.
We should be able to use SpanContext as reference descriptor for spans, but they end up holding way too much data.
So when I add to implement span links, I made sure to leave them in Span, not SpanContext.
I had a look at its Javadoc and it basically should only be IDs (trace + span), sampling, a bit of (W3C) trace state and baggage, nothing else :)
There was a problem hiding this comment.
Okay, that makes sense. I think that maybe we should repurpose SpanContext into SpanContents.
I have a bit of crazy idea to recycle SpanContents, but I want to do so while maintaining Span object identity. In other words, I'd always create a new Span, but the SpanContents inside the span would be reused.
There was a problem hiding this comment.
I've addressed the TagPostProcessor interaction by introducing two new interfaces: WritableSpanLinks and SpanLinkAccessor.
WritableSpanLinks just provides the ability to addLink, it is passed to TagPostProcessor. SpanLinkAccessor extends WritableSpanLinks and also provides a way to get the links.
DDSpan now implements SpanLinkAccessor. I picked this design, so I could avoid a capturing lambda around the span.
BenchmarksStartupParameters
See matching parameters
SummaryFound 0 performance improvements and 1 performance regressions! Performance is the same for 62 metrics, 8 unstable metrics.
Startup time reports for insecure-bankgantt
title insecure-bank - global startup overhead: candidate=1.61.0-SNAPSHOT~d145c52d6e, baseline=1.61.0-SNAPSHOT~847896ee54
dateFormat X
axisFormat %s
section tracing
Agent [baseline] (1.063 s) : 0, 1063373
Total [baseline] (8.877 s) : 0, 8877100
Agent [candidate] (1.061 s) : 0, 1061302
Total [candidate] (8.912 s) : 0, 8911904
section iast
Agent [baseline] (1.238 s) : 0, 1238496
Total [baseline] (9.611 s) : 0, 9611331
Agent [candidate] (1.228 s) : 0, 1228362
Total [candidate] (9.589 s) : 0, 9588994
gantt
title insecure-bank - break down per module: candidate=1.61.0-SNAPSHOT~d145c52d6e, baseline=1.61.0-SNAPSHOT~847896ee54
dateFormat X
axisFormat %s
section tracing
crashtracking [baseline] (1.222 ms) : 0, 1222
crashtracking [candidate] (1.202 ms) : 0, 1202
BytebuddyAgent [baseline] (634.156 ms) : 0, 634156
BytebuddyAgent [candidate] (633.364 ms) : 0, 633364
AgentMeter [baseline] (29.481 ms) : 0, 29481
AgentMeter [candidate] (29.416 ms) : 0, 29416
GlobalTracer [baseline] (257.532 ms) : 0, 257532
GlobalTracer [candidate] (257.502 ms) : 0, 257502
AppSec [baseline] (31.914 ms) : 0, 31914
AppSec [candidate] (31.872 ms) : 0, 31872
Debugger [baseline] (59.829 ms) : 0, 59829
Debugger [candidate] (59.628 ms) : 0, 59628
Remote Config [baseline] (609.249 µs) : 0, 609
Remote Config [candidate] (589.066 µs) : 0, 589
Telemetry [baseline] (8.126 ms) : 0, 8126
Telemetry [candidate] (8.039 ms) : 0, 8039
Flare Poller [baseline] (4.213 ms) : 0, 4213
Flare Poller [candidate] (3.509 ms) : 0, 3509
section iast
crashtracking [baseline] (1.196 ms) : 0, 1196
crashtracking [candidate] (1.19 ms) : 0, 1190
BytebuddyAgent [baseline] (803.575 ms) : 0, 803575
BytebuddyAgent [candidate] (796.081 ms) : 0, 796081
AgentMeter [baseline] (11.522 ms) : 0, 11522
AgentMeter [candidate] (11.357 ms) : 0, 11357
GlobalTracer [baseline] (250.165 ms) : 0, 250165
GlobalTracer [candidate] (247.607 ms) : 0, 247607
IAST [baseline] (25.661 ms) : 0, 25661
IAST [candidate] (25.574 ms) : 0, 25574
AppSec [baseline] (26.947 ms) : 0, 26947
AppSec [candidate] (26.737 ms) : 0, 26737
Debugger [baseline] (69.314 ms) : 0, 69314
Debugger [candidate] (68.99 ms) : 0, 68990
Remote Config [baseline] (539.88 µs) : 0, 540
Remote Config [candidate] (535.154 µs) : 0, 535
Telemetry [baseline] (9.674 ms) : 0, 9674
Telemetry [candidate] (10.331 ms) : 0, 10331
Flare Poller [baseline] (3.579 ms) : 0, 3579
Flare Poller [candidate] (3.825 ms) : 0, 3825
Startup time reports for petclinicgantt
title petclinic - global startup overhead: candidate=1.61.0-SNAPSHOT~d145c52d6e, baseline=1.61.0-SNAPSHOT~847896ee54
dateFormat X
axisFormat %s
section tracing
Agent [baseline] (1.061 s) : 0, 1060556
Total [baseline] (11.063 s) : 0, 11062972
Agent [candidate] (1.068 s) : 0, 1067626
Total [candidate] (11.142 s) : 0, 11141586
section appsec
Agent [baseline] (1.265 s) : 0, 1264723
Total [baseline] (11.155 s) : 0, 11154821
Agent [candidate] (1.255 s) : 0, 1254714
Total [candidate] (11.144 s) : 0, 11143655
section iast
Agent [baseline] (1.239 s) : 0, 1239034
Total [baseline] (11.308 s) : 0, 11307851
Agent [candidate] (1.247 s) : 0, 1247477
Total [candidate] (11.303 s) : 0, 11303278
section profiling
Agent [baseline] (1.19 s) : 0, 1189745
Total [baseline] (11.035 s) : 0, 11034802
Agent [candidate] (1.185 s) : 0, 1184767
Total [candidate] (11.071 s) : 0, 11070889
gantt
title petclinic - break down per module: candidate=1.61.0-SNAPSHOT~d145c52d6e, baseline=1.61.0-SNAPSHOT~847896ee54
dateFormat X
axisFormat %s
section tracing
crashtracking [baseline] (1.191 ms) : 0, 1191
crashtracking [candidate] (1.217 ms) : 0, 1217
BytebuddyAgent [baseline] (630.674 ms) : 0, 630674
BytebuddyAgent [candidate] (635.177 ms) : 0, 635177
AgentMeter [baseline] (29.483 ms) : 0, 29483
AgentMeter [candidate] (29.779 ms) : 0, 29779
GlobalTracer [baseline] (257.49 ms) : 0, 257490
GlobalTracer [candidate] (259.26 ms) : 0, 259260
AppSec [baseline] (31.945 ms) : 0, 31945
AppSec [candidate] (32.129 ms) : 0, 32129
Debugger [baseline] (60.697 ms) : 0, 60697
Debugger [candidate] (60.856 ms) : 0, 60856
Remote Config [baseline] (589.345 µs) : 0, 589
Remote Config [candidate] (604.738 µs) : 0, 605
Telemetry [baseline] (8.052 ms) : 0, 8052
Telemetry [candidate] (8.864 ms) : 0, 8864
Flare Poller [baseline] (4.33 ms) : 0, 4330
Flare Poller [candidate] (3.546 ms) : 0, 3546
section appsec
crashtracking [baseline] (1.207 ms) : 0, 1207
crashtracking [candidate] (1.208 ms) : 0, 1208
BytebuddyAgent [baseline] (671.451 ms) : 0, 671451
BytebuddyAgent [candidate] (664.662 ms) : 0, 664662
AgentMeter [baseline] (12.211 ms) : 0, 12211
AgentMeter [candidate] (12.101 ms) : 0, 12101
GlobalTracer [baseline] (261.434 ms) : 0, 261434
GlobalTracer [candidate] (258.953 ms) : 0, 258953
AppSec [baseline] (178.451 ms) : 0, 178451
AppSec [candidate] (178.069 ms) : 0, 178069
Debugger [baseline] (66.66 ms) : 0, 66660
Debugger [candidate] (66.451 ms) : 0, 66451
Remote Config [baseline] (636.54 µs) : 0, 637
Remote Config [candidate] (679.128 µs) : 0, 679
Telemetry [baseline] (8.29 ms) : 0, 8290
Telemetry [candidate] (8.443 ms) : 0, 8443
Flare Poller [baseline] (3.595 ms) : 0, 3595
Flare Poller [candidate] (3.564 ms) : 0, 3564
IAST [baseline] (24.367 ms) : 0, 24367
IAST [candidate] (24.203 ms) : 0, 24203
section iast
crashtracking [baseline] (1.22 ms) : 0, 1220
crashtracking [candidate] (1.242 ms) : 0, 1242
BytebuddyAgent [baseline] (804.729 ms) : 0, 804729
BytebuddyAgent [candidate] (811.628 ms) : 0, 811628
AgentMeter [baseline] (11.668 ms) : 0, 11668
AgentMeter [candidate] (12.005 ms) : 0, 12005
GlobalTracer [baseline] (249.155 ms) : 0, 249155
GlobalTracer [candidate] (249.582 ms) : 0, 249582
AppSec [baseline] (26.676 ms) : 0, 26676
AppSec [candidate] (26.796 ms) : 0, 26796
Debugger [baseline] (70.026 ms) : 0, 70026
Debugger [candidate] (68.933 ms) : 0, 68933
Remote Config [baseline] (537.911 µs) : 0, 538
Remote Config [candidate] (536.393 µs) : 0, 536
Telemetry [baseline] (9.582 ms) : 0, 9582
Telemetry [candidate] (10.922 ms) : 0, 10922
Flare Poller [baseline] (3.577 ms) : 0, 3577
Flare Poller [candidate] (3.755 ms) : 0, 3755
IAST [baseline] (25.57 ms) : 0, 25570
IAST [candidate] (25.601 ms) : 0, 25601
section profiling
ProfilingAgent [baseline] (94.716 ms) : 0, 94716
ProfilingAgent [candidate] (94.169 ms) : 0, 94169
crashtracking [baseline] (1.168 ms) : 0, 1168
crashtracking [candidate] (1.192 ms) : 0, 1192
BytebuddyAgent [baseline] (686.512 ms) : 0, 686512
BytebuddyAgent [candidate] (684.249 ms) : 0, 684249
AgentMeter [baseline] (9.062 ms) : 0, 9062
AgentMeter [candidate] (8.995 ms) : 0, 8995
GlobalTracer [baseline] (216.394 ms) : 0, 216394
GlobalTracer [candidate] (214.785 ms) : 0, 214785
AppSec [baseline] (32.521 ms) : 0, 32521
AppSec [candidate] (32.266 ms) : 0, 32266
Debugger [baseline] (66.422 ms) : 0, 66422
Debugger [candidate] (66.162 ms) : 0, 66162
Remote Config [baseline] (574.863 µs) : 0, 575
Remote Config [candidate] (586.496 µs) : 0, 586
Telemetry [baseline] (7.824 ms) : 0, 7824
Telemetry [candidate] (7.797 ms) : 0, 7797
Flare Poller [baseline] (3.561 ms) : 0, 3561
Flare Poller [candidate] (3.581 ms) : 0, 3581
Profiling [baseline] (95.27 ms) : 0, 95270
Profiling [candidate] (94.723 ms) : 0, 94723
LoadParameters
See matching parameters
SummaryFound 0 performance improvements and 0 performance regressions! Performance is the same for 20 metrics, 16 unstable metrics. Request duration reports for insecure-bankgantt
title insecure-bank - request duration [CI 0.99] : candidate=1.61.0-SNAPSHOT~d145c52d6e, baseline=1.61.0-SNAPSHOT~847896ee54
dateFormat X
axisFormat %s
section baseline
no_agent (1.229 ms) : 1218, 1241
. : milestone, 1229,
iast (3.364 ms) : 3319, 3408
. : milestone, 3364,
iast_FULL (6.09 ms) : 6027, 6153
. : milestone, 6090,
iast_GLOBAL (3.82 ms) : 3754, 3885
. : milestone, 3820,
profiling (2.375 ms) : 2351, 2399
. : milestone, 2375,
tracing (1.878 ms) : 1862, 1894
. : milestone, 1878,
section candidate
no_agent (1.24 ms) : 1229, 1252
. : milestone, 1240,
iast (3.312 ms) : 3270, 3353
. : milestone, 3312,
iast_FULL (6.052 ms) : 5990, 6114
. : milestone, 6052,
iast_GLOBAL (3.832 ms) : 3770, 3894
. : milestone, 3832,
profiling (2.18 ms) : 2160, 2199
. : milestone, 2180,
tracing (1.956 ms) : 1938, 1973
. : milestone, 1956,
Request duration reports for petclinicgantt
title petclinic - request duration [CI 0.99] : candidate=1.61.0-SNAPSHOT~d145c52d6e, baseline=1.61.0-SNAPSHOT~847896ee54
dateFormat X
axisFormat %s
section baseline
no_agent (18.249 ms) : 18062, 18435
. : milestone, 18249,
appsec (18.63 ms) : 18443, 18817
. : milestone, 18630,
code_origins (17.864 ms) : 17688, 18039
. : milestone, 17864,
iast (18.008 ms) : 17830, 18186
. : milestone, 18008,
profiling (18.457 ms) : 18273, 18641
. : milestone, 18457,
tracing (17.765 ms) : 17591, 17939
. : milestone, 17765,
section candidate
no_agent (18.315 ms) : 18128, 18502
. : milestone, 18315,
appsec (18.644 ms) : 18456, 18831
. : milestone, 18644,
code_origins (17.619 ms) : 17446, 17793
. : milestone, 17619,
iast (18.186 ms) : 18004, 18367
. : milestone, 18186,
profiling (18.806 ms) : 18618, 18994
. : milestone, 18806,
tracing (17.958 ms) : 17781, 18136
. : milestone, 17958,
DacapoParameters
See matching parameters
SummaryFound 1 performance improvements and 0 performance regressions! Performance is the same for 11 metrics, 0 unstable metrics.
Execution time for biojavagantt
title biojava - execution time [CI 0.99] : candidate=1.61.0-SNAPSHOT~d145c52d6e, baseline=1.61.0-SNAPSHOT~847896ee54
dateFormat X
axisFormat %s
section baseline
no_agent (15.075 s) : 15075000, 15075000
. : milestone, 15075000,
appsec (14.805 s) : 14805000, 14805000
. : milestone, 14805000,
iast (18.275 s) : 18275000, 18275000
. : milestone, 18275000,
iast_GLOBAL (17.833 s) : 17833000, 17833000
. : milestone, 17833000,
profiling (14.97 s) : 14970000, 14970000
. : milestone, 14970000,
tracing (15.213 s) : 15213000, 15213000
. : milestone, 15213000,
section candidate
no_agent (15.656 s) : 15656000, 15656000
. : milestone, 15656000,
appsec (15.034 s) : 15034000, 15034000
. : milestone, 15034000,
iast (18.434 s) : 18434000, 18434000
. : milestone, 18434000,
iast_GLOBAL (17.912 s) : 17912000, 17912000
. : milestone, 17912000,
profiling (14.925 s) : 14925000, 14925000
. : milestone, 14925000,
tracing (15.052 s) : 15052000, 15052000
. : milestone, 15052000,
Execution time for tomcatgantt
title tomcat - execution time [CI 0.99] : candidate=1.61.0-SNAPSHOT~d145c52d6e, baseline=1.61.0-SNAPSHOT~847896ee54
dateFormat X
axisFormat %s
section baseline
no_agent (1.486 ms) : 1475, 1498
. : milestone, 1486,
appsec (3.822 ms) : 3601, 4042
. : milestone, 3822,
iast (2.266 ms) : 2198, 2335
. : milestone, 2266,
iast_GLOBAL (2.305 ms) : 2236, 2374
. : milestone, 2305,
profiling (2.113 ms) : 2057, 2169
. : milestone, 2113,
tracing (2.075 ms) : 2022, 2128
. : milestone, 2075,
section candidate
no_agent (1.479 ms) : 1468, 1491
. : milestone, 1479,
appsec (2.537 ms) : 2482, 2591
. : milestone, 2537,
iast (2.269 ms) : 2200, 2338
. : milestone, 2269,
iast_GLOBAL (2.304 ms) : 2234, 2373
. : milestone, 2304,
profiling (2.087 ms) : 2032, 2141
. : milestone, 2087,
tracing (2.073 ms) : 2020, 2127
. : milestone, 2073,
|
Introduced the new interface WritableSpanLinks & SpanLinkAccessor that are used to provide limited access to DDSpan Updated TagProcessors to use the new interfaces Updated tests as needed
…trace-java into dougqh/span-links-allocation
| @Override | ||
| public void processTags( | ||
| TagMap unsafeTags, DDSpanContext spanContext, List<AgentSpanLink> spanLinks) { | ||
| TagMap unsafeTags, DDSpanContext spanContext, WritableSpanLinks spanLinks) { |
There was a problem hiding this comment.
I introduced WritableSpanLinks as way to update the span links inside of TagPostProcessors.
The reason for the new interface is that I wanted to avoid creating a capturing lambda around the DDSpan, so instead I have DDSpan implement WritableSpanLinks.
| @@ -12,12 +13,12 @@ public abstract class TagsPostProcessor { | |||
| */ | |||
| @Deprecated | |||
| final Map<String, Object> processTags( | |||
There was a problem hiding this comment.
Before I merge this PR, I'm going to get rid of this vestige of the old signature. I just have to update a bunch of tests, but I'm already touching a bunch of those files anyway.
| @Override | ||
| public void processServiceTags() { | ||
| context.earlyProcessTags(links); | ||
| context.earlyProcessTags(this); |
There was a problem hiding this comment.
this DDSpan is now a SpanLinkAccessor / WritableSpanLinks
There was a problem hiding this comment.
Updated - now just an AppendableSpanLinks (previously WritableSpanLinks)
SpanLinkAccessor has been removed
| @Override | ||
| public void processTagsAndBaggage(final MetadataConsumer consumer) { | ||
| context.processTagsAndBaggage(consumer, longRunningVersion, links); | ||
| context.processTagsAndBaggage(consumer, longRunningVersion, this); |
There was a problem hiding this comment.
this DDSpan is now a SpanLinkAccessor / WritableSpanLinks
There was a problem hiding this comment.
Updated - now just an AppendableSpanLinks (previously WritableSpanLinks)
SpanLinkAccessor has been removed
| } | ||
|
|
||
| @Override | ||
| public List<? extends AgentSpanLink> getLinks() { |
There was a problem hiding this comment.
I don't entirely like that I had to add this public method, but I took care to make the signature ? extends AgentSpanLink to discourage writing to the list directly.
There was a problem hiding this comment.
Same, I don't like exposing the links. It was a strong design constraints when I added span links.
The tags and baggage handling are already so messy, I don't want to expose the span links collections.
Additionally, the Javadoc for this method states the collection should be an immutable collection where the return collection is mutable. That's even worst if we want to prevent to 1. show the internal span links implementation 2. contributors to modify span links outside DDSpan.
I have open a Slack discussion about fixing the root design issue before trying to unlocking more public API.
There was a problem hiding this comment.
Yes, I didn't add an immutable wrapper that would negate the some of the gains from this change.
The reason that I called it immutable is because the specific type of ? extends AgentSpanLink prevents calling most of the modification methods without casting.
For now, I think the best option might be to just drop the SpanLinkAccessor interface and pass DDSpan directly to DDSpanContext. I was trying to restrict what DDSpanContext touches during processing, but the trade-off of adding a public getLinks might not be worth it.
There was a problem hiding this comment.
For now, I think the best option might be to just drop the
SpanLinkAccessorinterface and passDDSpandirectly toDDSpanContext. I was trying to restrict whatDDSpanContexttouches during processing, but the trade-off of adding a publicgetLinksmight not be worth it.
That would indeed "solve" the point I raised here #10822 (comment)
| * @return The encoded tag value, {@code null} if no links. | ||
| */ | ||
| public static String toTag(List<AgentSpanLink> links) { | ||
| public static String toTag(List<? extends AgentSpanLink> links) { |
There was a problem hiding this comment.
Updated to accommodate return from SpanLinkAccessor.getLinks
amarziali
left a comment
amarziali
left a comment
There was a problem hiding this comment.
LGTM thanks for the extra refactoring. Now postprocessors looks safer
internal-api/src/main/java/datadog/trace/bootstrap/instrumentation/api/WritableSpanLinks.java
Outdated
Show resolved
Hide resolved
dd-trace-core/src/main/java/datadog/trace/core/SpanLinkAccessor.java
Outdated
Show resolved
Hide resolved
internal-api/src/main/java/datadog/trace/bootstrap/instrumentation/api/WritableSpanLinks.java
Outdated
Show resolved
Hide resolved
dd-trace-core/src/main/java/datadog/trace/core/SpanLinkAccessor.java
Outdated
Show resolved
Hide resolved
- removing implied visibility from new interfaces - added some javadoc to new interfaces - renamed WritableSpanLinks -> AppendableSpanLinks
…gsPostProcessor Removed vestigial processTags(Map, DDSpanContext, List) method from TagsPostProcessor This method was a hold over from before TagMap was introduced. It was kept to make porting tests easier while TagMap was still in experimental phase. The method has outlived its usefulness; however, removing the method did require updating a lot of tests.
|
Waiting for review about #10974 |
I took a look. I do think that approach is a cleaner, but I don't want us to move work into instrumentation / application thread. I really want us to be doing the opposite. Admittedly, I don't think TagPostProcessor provides a great API for doing background work on a span, but I also don't want to take a step back performance wise. And that's doubly true for span links which have a heavy weight construction process at the moment. |
Removed SpanLinkAccessor interface SpanLinkAccessor was intended to limit how the span is modified in DDSpanContext#processTagsAndBaggage, but the trade-off was introducing a public getLinks method on DDSpan. Reduced visibility of processTagsAndBaggage methods on DDSpanContext Since processTagsAndBaggage is really just a helper routine of DDSpan.processTagsAndBaggage and the usage is a bit sensitive, I decided to reduce the visibility
| return context.getTraceCollector().getTraceConfig(); | ||
| } | ||
|
|
||
| List<? extends AgentSpanLink> getLinks() { |
There was a problem hiding this comment.
I've kept this accessor, but it is now only package visible.
It was previously public because of needing to implement the SpanLinksAccessor interface which has now been removed.
| } | ||
|
|
||
| public void earlyProcessTags(List<AgentSpanLink> links) { | ||
| void earlyProcessTags(AppendableSpanLinks links) { |
There was a problem hiding this comment.
Reduced the visibility of earlyProcessTags and processTagsAndBaggage
As far as I can tell, these are really just helper methods of DDSpan
| final MetadataConsumer consumer, int longRunningVersion, List<AgentSpanLink> links) { | ||
| void processTagsAndBaggage( | ||
| final MetadataConsumer consumer, int longRunningVersion, DDSpan restrictedSpan) { | ||
| // NOTE: The span is passed for the sole purpose of allowing updating & reading of the span links |
There was a problem hiding this comment.
The name restrictedSpan and the comment are to try and dissuade others from doing unwanted things with span. That's also why I originally created SpanLinksAccessor, but that required making getLinks public which was arguably worse.
| import java.util.Map; | ||
|
|
||
| public abstract class TagsPostProcessor { | ||
| /* |
There was a problem hiding this comment.
Got rid of the old processTags which I had kept around for test migration.
That does mean that I had to update a lot of tests.
| def processor = new HttpEndpointPostProcessor(endpointResolver) | ||
| def mockContext = Mock(DDSpanContext) | ||
| def tags = [ | ||
| def mockSpanLinks = Mock(AppendableSpanLinks) |
There was a problem hiding this comment.
Lots of test updates, I tried to keep the changes as mechanical and straight forward as possible
PerfectSlayer
left a comment
PerfectSlayer
left a comment
There was a problem hiding this comment.
Thanks for the discussions and the follow up changes.
We definitely need a better API for "post processing" / "lazy computation"... And maybe not only at span level.
| TagsPostProcessorFactory.lazyProcessor().processTags(unsafeTags, this, restrictedSpan); | ||
|
|
||
| String linksTag = DDSpanLink.toTag(links); | ||
| String linksTag = DDSpanLink.toTag(restrictedSpan.getLinks()); |
There was a problem hiding this comment.
🎯 suggestion: We don't even need to introduce getLinks() if you want to keep the changes minimal as the links field is already accessible from this place:
| String linksTag = DDSpanLink.toTag(restrictedSpan.getLinks()); | |
| String linksTag = DDSpanLink.toTag(restrictedSpan.links); |
|
Should I close #10974 then? |
|
/merge |
|
View all feedbacks in Devflow UI.
The expected merge time in
|
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
gh-worker-dd-mergequeue-cf854d
bot
deleted the
dougqh/span-links-allocation
branch
5 participants
What Does This Do
This change shares an "EMPTY" list between spans when there are no span links.
This avoids the construction of a CopyOnWriteArrayList, empty Object[], and internal lock Object for the common case where there are no links.
Motivation
This elimination of the CopyOnWriteArrayList in the empty case, reduced allocation by 10 GiB / 5% and GC time by 7% in a span creation stress test.
Additional Notes
Original change overlooked the interaction with TagPostProcessors.
To solve that, the revised approach introduces AppendableSpanLinks as an interface passed to TagPostProcessors instead of List.
And to be extra safe, also introduce SpanLinkAccessor for use by DDSpanContext.processTags.
In both cases, the implementation provided when called is just the DDSpan, but the restricted interface discourages making other changes through DDSpan directly.
Contributor Checklist
type:and (comp:orinst:) labels in addition to any other useful labelsclose,fix, or any linking keywords when referencing an issueUse
solvesinstead, and assign the PR milestone to the issueJira ticket: [PROJ-IDENT]
Note: Once your PR is ready to merge, add it to the merge queue by commenting
/merge./merge -ccancels the queue request./merge -f --reason "reason"skips all merge queue checks; please use this judiciously, as some checks do not run at the PR-level. For more information, see this doc.