Avoid creation of empty CopyOnWriteArrayList for span links#10822
Avoid creation of empty CopyOnWriteArrayList for span links#10822
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 0 performance regressions! Performance is the same for 64 metrics, 7 unstable metrics. Startup time reports for insecure-bankgantt
title insecure-bank - global startup overhead: candidate=1.61.0-SNAPSHOT~ca95b4294d, baseline=1.61.0-SNAPSHOT~c0ce9c5738
dateFormat X
axisFormat %s
section tracing
Agent [baseline] (1.06 s) : 0, 1059632
Total [baseline] (8.882 s) : 0, 8881765
Agent [candidate] (1.058 s) : 0, 1058287
Total [candidate] (8.86 s) : 0, 8860208
section iast
Agent [baseline] (1.235 s) : 0, 1234926
Total [baseline] (9.589 s) : 0, 9589450
Agent [candidate] (1.227 s) : 0, 1227112
Total [candidate] (9.557 s) : 0, 9557246
gantt
title insecure-bank - break down per module: candidate=1.61.0-SNAPSHOT~ca95b4294d, baseline=1.61.0-SNAPSHOT~c0ce9c5738
dateFormat X
axisFormat %s
section tracing
crashtracking [baseline] (1.194 ms) : 0, 1194
crashtracking [candidate] (1.202 ms) : 0, 1202
BytebuddyAgent [baseline] (630.252 ms) : 0, 630252
BytebuddyAgent [candidate] (629.114 ms) : 0, 629114
AgentMeter [baseline] (29.414 ms) : 0, 29414
AgentMeter [candidate] (29.394 ms) : 0, 29394
GlobalTracer [baseline] (257.579 ms) : 0, 257579
GlobalTracer [candidate] (256.882 ms) : 0, 256882
AppSec [baseline] (31.812 ms) : 0, 31812
AppSec [candidate] (31.773 ms) : 0, 31773
Debugger [baseline] (59.572 ms) : 0, 59572
Debugger [candidate] (59.466 ms) : 0, 59466
Remote Config [baseline] (595.956 µs) : 0, 596
Remote Config [candidate] (585.958 µs) : 0, 586
Telemetry [baseline] (8.046 ms) : 0, 8046
Telemetry [candidate] (8.104 ms) : 0, 8104
Flare Poller [baseline] (5.086 ms) : 0, 5086
Flare Poller [candidate] (5.697 ms) : 0, 5697
section iast
crashtracking [baseline] (1.202 ms) : 0, 1202
crashtracking [candidate] (1.193 ms) : 0, 1193
BytebuddyAgent [baseline] (801.243 ms) : 0, 801243
BytebuddyAgent [candidate] (796.195 ms) : 0, 796195
AgentMeter [baseline] (11.503 ms) : 0, 11503
AgentMeter [candidate] (11.413 ms) : 0, 11413
GlobalTracer [baseline] (249.221 ms) : 0, 249221
GlobalTracer [candidate] (247.424 ms) : 0, 247424
AppSec [baseline] (26.813 ms) : 0, 26813
AppSec [candidate] (26.527 ms) : 0, 26527
Debugger [baseline] (69.332 ms) : 0, 69332
Debugger [candidate] (68.517 ms) : 0, 68517
Remote Config [baseline] (533.448 µs) : 0, 533
Remote Config [candidate] (532.062 µs) : 0, 532
Telemetry [baseline] (9.769 ms) : 0, 9769
Telemetry [candidate] (10.293 ms) : 0, 10293
Flare Poller [baseline] (3.44 ms) : 0, 3440
Flare Poller [candidate] (3.582 ms) : 0, 3582
IAST [baseline] (25.552 ms) : 0, 25552
IAST [candidate] (25.251 ms) : 0, 25251
Startup time reports for petclinicgantt
title petclinic - global startup overhead: candidate=1.61.0-SNAPSHOT~ca95b4294d, baseline=1.61.0-SNAPSHOT~c0ce9c5738
dateFormat X
axisFormat %s
section tracing
Agent [baseline] (1.07 s) : 0, 1070137
Total [baseline] (11.214 s) : 0, 11213681
Agent [candidate] (1.057 s) : 0, 1056820
Total [candidate] (11.156 s) : 0, 11155647
section appsec
Agent [baseline] (1.256 s) : 0, 1255832
Total [baseline] (11.157 s) : 0, 11157228
Agent [candidate] (1.248 s) : 0, 1247650
Total [candidate] (11.065 s) : 0, 11065340
section iast
Agent [baseline] (1.232 s) : 0, 1232174
Total [baseline] (11.292 s) : 0, 11292211
Agent [candidate] (1.238 s) : 0, 1237565
Total [candidate] (11.298 s) : 0, 11298417
section profiling
Agent [baseline] (1.196 s) : 0, 1195951
Total [baseline] (11.212 s) : 0, 11211981
Agent [candidate] (1.184 s) : 0, 1183743
Total [candidate] (10.973 s) : 0, 10972630
gantt
title petclinic - break down per module: candidate=1.61.0-SNAPSHOT~ca95b4294d, baseline=1.61.0-SNAPSHOT~c0ce9c5738
dateFormat X
axisFormat %s
section tracing
crashtracking [baseline] (1.204 ms) : 0, 1204
crashtracking [candidate] (1.199 ms) : 0, 1199
BytebuddyAgent [baseline] (638.167 ms) : 0, 638167
BytebuddyAgent [candidate] (627.485 ms) : 0, 627485
AgentMeter [baseline] (29.643 ms) : 0, 29643
AgentMeter [candidate] (29.364 ms) : 0, 29364
GlobalTracer [baseline] (258.768 ms) : 0, 258768
GlobalTracer [candidate] (256.547 ms) : 0, 256547
AppSec [baseline] (32.034 ms) : 0, 32034
AppSec [candidate] (31.772 ms) : 0, 31772
Debugger [baseline] (61.012 ms) : 0, 61012
Debugger [candidate] (60.164 ms) : 0, 60164
Remote Config [baseline] (593.741 µs) : 0, 594
Remote Config [candidate] (581.178 µs) : 0, 581
Telemetry [baseline] (8.911 ms) : 0, 8911
Telemetry [candidate] (8.775 ms) : 0, 8775
Flare Poller [baseline] (3.588 ms) : 0, 3588
Flare Poller [candidate] (5.007 ms) : 0, 5007
section appsec
crashtracking [baseline] (1.2 ms) : 0, 1200
crashtracking [candidate] (1.187 ms) : 0, 1187
BytebuddyAgent [baseline] (663.19 ms) : 0, 663190
BytebuddyAgent [candidate] (659.224 ms) : 0, 659224
AgentMeter [baseline] (12.283 ms) : 0, 12283
AgentMeter [candidate] (12.073 ms) : 0, 12073
GlobalTracer [baseline] (259.979 ms) : 0, 259979
GlobalTracer [candidate] (257.85 ms) : 0, 257850
AppSec [baseline] (178.889 ms) : 0, 178889
AppSec [candidate] (177.728 ms) : 0, 177728
Debugger [baseline] (66.744 ms) : 0, 66744
Debugger [candidate] (66.453 ms) : 0, 66453
Remote Config [baseline] (642.305 µs) : 0, 642
Remote Config [candidate] (636.713 µs) : 0, 637
Telemetry [baseline] (8.443 ms) : 0, 8443
Telemetry [candidate] (8.471 ms) : 0, 8471
Flare Poller [baseline] (3.655 ms) : 0, 3655
Flare Poller [candidate] (3.649 ms) : 0, 3649
IAST [baseline] (24.423 ms) : 0, 24423
IAST [candidate] (24.169 ms) : 0, 24169
section iast
crashtracking [baseline] (1.201 ms) : 0, 1201
crashtracking [candidate] (1.201 ms) : 0, 1201
BytebuddyAgent [baseline] (799.528 ms) : 0, 799528
BytebuddyAgent [candidate] (804.46 ms) : 0, 804460
AgentMeter [baseline] (11.452 ms) : 0, 11452
AgentMeter [candidate] (11.554 ms) : 0, 11554
GlobalTracer [baseline] (248.28 ms) : 0, 248280
GlobalTracer [candidate] (248.525 ms) : 0, 248525
AppSec [baseline] (26.698 ms) : 0, 26698
AppSec [candidate] (26.619 ms) : 0, 26619
Debugger [baseline] (69.799 ms) : 0, 69799
Debugger [candidate] (69.881 ms) : 0, 69881
Remote Config [baseline] (530.556 µs) : 0, 531
Remote Config [candidate] (526.093 µs) : 0, 526
Telemetry [baseline] (9.705 ms) : 0, 9705
Telemetry [candidate] (9.626 ms) : 0, 9626
Flare Poller [baseline] (3.547 ms) : 0, 3547
Flare Poller [candidate] (3.508 ms) : 0, 3508
IAST [baseline] (25.338 ms) : 0, 25338
IAST [candidate] (25.338 ms) : 0, 25338
section profiling
crashtracking [baseline] (1.195 ms) : 0, 1195
crashtracking [candidate] (1.168 ms) : 0, 1168
BytebuddyAgent [baseline] (691.019 ms) : 0, 691019
BytebuddyAgent [candidate] (684.252 ms) : 0, 684252
AgentMeter [baseline] (9.021 ms) : 0, 9021
AgentMeter [candidate] (9.073 ms) : 0, 9073
GlobalTracer [baseline] (216.575 ms) : 0, 216575
GlobalTracer [candidate] (214.737 ms) : 0, 214737
AppSec [baseline] (32.389 ms) : 0, 32389
AppSec [candidate] (32.254 ms) : 0, 32254
Debugger [baseline] (66.096 ms) : 0, 66096
Debugger [candidate] (65.811 ms) : 0, 65811
Remote Config [baseline] (587.994 µs) : 0, 588
Remote Config [candidate] (555.789 µs) : 0, 556
Telemetry [baseline] (7.943 ms) : 0, 7943
Telemetry [candidate] (7.748 ms) : 0, 7748
Flare Poller [baseline] (3.571 ms) : 0, 3571
Flare Poller [candidate] (3.471 ms) : 0, 3471
ProfilingAgent [baseline] (95.516 ms) : 0, 95516
ProfilingAgent [candidate] (93.79 ms) : 0, 93790
Profiling [baseline] (96.088 ms) : 0, 96088
Profiling [candidate] (94.344 ms) : 0, 94344
LoadParameters
See matching parameters
SummaryFound 3 performance improvements and 2 performance regressions! Performance is the same for 16 metrics, 15 unstable metrics.
Request duration reports for insecure-bankgantt
title insecure-bank - request duration [CI 0.99] : candidate=1.61.0-SNAPSHOT~ca95b4294d, baseline=1.61.0-SNAPSHOT~c0ce9c5738
dateFormat X
axisFormat %s
section baseline
no_agent (1.201 ms) : 1189, 1213
. : milestone, 1201,
iast (3.282 ms) : 3237, 3328
. : milestone, 3282,
iast_FULL (6.015 ms) : 5954, 6076
. : milestone, 6015,
iast_GLOBAL (3.536 ms) : 3479, 3593
. : milestone, 3536,
profiling (2.058 ms) : 2039, 2077
. : milestone, 2058,
tracing (1.779 ms) : 1764, 1793
. : milestone, 1779,
section candidate
no_agent (1.214 ms) : 1202, 1226
. : milestone, 1214,
iast (3.217 ms) : 3173, 3261
. : milestone, 3217,
iast_FULL (5.763 ms) : 5706, 5821
. : milestone, 5763,
iast_GLOBAL (3.537 ms) : 3475, 3598
. : milestone, 3537,
profiling (2.166 ms) : 2146, 2186
. : milestone, 2166,
tracing (1.854 ms) : 1838, 1870
. : milestone, 1854,
Request duration reports for petclinicgantt
title petclinic - request duration [CI 0.99] : candidate=1.61.0-SNAPSHOT~ca95b4294d, baseline=1.61.0-SNAPSHOT~c0ce9c5738
dateFormat X
axisFormat %s
section baseline
no_agent (18.793 ms) : 18601, 18985
. : milestone, 18793,
appsec (18.726 ms) : 18539, 18914
. : milestone, 18726,
code_origins (17.737 ms) : 17561, 17913
. : milestone, 17737,
iast (17.88 ms) : 17701, 18059
. : milestone, 17880,
profiling (18.285 ms) : 18104, 18466
. : milestone, 18285,
tracing (17.783 ms) : 17609, 17957
. : milestone, 17783,
section candidate
no_agent (16.939 ms) : 16773, 17105
. : milestone, 16939,
appsec (18.629 ms) : 18440, 18818
. : milestone, 18629,
code_origins (17.575 ms) : 17399, 17751
. : milestone, 17575,
iast (17.78 ms) : 17600, 17959
. : milestone, 17780,
profiling (19.585 ms) : 19379, 19792
. : milestone, 19585,
tracing (17.661 ms) : 17488, 17835
. : milestone, 17661,
DacapoParameters
See matching parameters
SummaryFound 0 performance improvements and 0 performance regressions! Performance is the same for 11 metrics, 1 unstable metrics. Execution time for tomcatgantt
title tomcat - execution time [CI 0.99] : candidate=1.61.0-SNAPSHOT~ca95b4294d, baseline=1.61.0-SNAPSHOT~c0ce9c5738
dateFormat X
axisFormat %s
section baseline
no_agent (1.47 ms) : 1458, 1481
. : milestone, 1470,
appsec (3.721 ms) : 3504, 3938
. : milestone, 3721,
iast (2.244 ms) : 2174, 2313
. : milestone, 2244,
iast_GLOBAL (2.294 ms) : 2224, 2364
. : milestone, 2294,
profiling (2.088 ms) : 2032, 2145
. : milestone, 2088,
tracing (2.059 ms) : 2005, 2113
. : milestone, 2059,
section candidate
no_agent (1.469 ms) : 1457, 1480
. : milestone, 1469,
appsec (3.789 ms) : 3568, 4010
. : milestone, 3789,
iast (2.25 ms) : 2181, 2319
. : milestone, 2250,
iast_GLOBAL (2.295 ms) : 2225, 2365
. : milestone, 2295,
profiling (2.073 ms) : 2018, 2128
. : milestone, 2073,
tracing (2.056 ms) : 2002, 2110
. : milestone, 2056,
Execution time for biojavagantt
title biojava - execution time [CI 0.99] : candidate=1.61.0-SNAPSHOT~ca95b4294d, baseline=1.61.0-SNAPSHOT~c0ce9c5738
dateFormat X
axisFormat %s
section baseline
no_agent (15.52 s) : 15520000, 15520000
. : milestone, 15520000,
appsec (14.8 s) : 14800000, 14800000
. : milestone, 14800000,
iast (17.914 s) : 17914000, 17914000
. : milestone, 17914000,
iast_GLOBAL (17.589 s) : 17589000, 17589000
. : milestone, 17589000,
profiling (14.881 s) : 14881000, 14881000
. : milestone, 14881000,
tracing (14.675 s) : 14675000, 14675000
. : milestone, 14675000,
section candidate
no_agent (15.463 s) : 15463000, 15463000
. : milestone, 15463000,
appsec (14.66 s) : 14660000, 14660000
. : milestone, 14660000,
iast (18.386 s) : 18386000, 18386000
. : milestone, 18386000,
iast_GLOBAL (17.875 s) : 17875000, 17875000
. : milestone, 17875000,
profiling (15.321 s) : 15321000, 15321000
. : milestone, 15321000,
tracing (14.839 s) : 14839000, 14839000
. : milestone, 14839000,
|
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
4 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.