Replacing interceptors ConcurrentSkipListSet with priority ordered copy on write "list"#11002
Conversation
…n write "list" The ConcurrentSkipListSet iteration was sometimes adding 7 GiB (out 250 GiB) in span creation stress test. Probably from escape analysis sporadically failing based on inlining decisions. ConcurrentSkipListSet was more than needed for this particular problem. Adding to the list is rare, so a copy on write "list" seemed like a better fit. So I introduced TraceInterceptors which serves as a copy on write "list", but where add is doing through insertion sort.
|
Hi! 👋 Thanks for your pull request! 🎉 To help us review it, please make sure to:
If you need help, please check our contributing guidelines. |
codeNarc groovy imports code reshuffling
…ataDog/dd-trace-java into dougqh/trace-interceptors-optimization
BenchmarksStartupParameters
See matching parameters
SummaryFound 2 performance improvements and 0 performance regressions! Performance is the same for 60 metrics, 9 unstable metrics.
Startup time reports for petclinicgantt
title petclinic - global startup overhead: candidate=1.61.0-SNAPSHOT~0d5f341a1a, baseline=1.61.0-SNAPSHOT~0ee26e896c
dateFormat X
axisFormat %s
section tracing
Agent [baseline] (1.07 s) : 0, 1069709
Total [baseline] (11.137 s) : 0, 11136747
Agent [candidate] (1.067 s) : 0, 1067004
Total [candidate] (11.104 s) : 0, 11104128
section appsec
Agent [baseline] (1.251 s) : 0, 1251269
Total [baseline] (11.233 s) : 0, 11233400
Agent [candidate] (1.249 s) : 0, 1249095
Total [candidate] (11.176 s) : 0, 11176268
section iast
Agent [baseline] (1.241 s) : 0, 1240915
Total [baseline] (11.339 s) : 0, 11339042
Agent [candidate] (1.236 s) : 0, 1236458
Total [candidate] (11.256 s) : 0, 11256408
section profiling
Agent [baseline] (1.191 s) : 0, 1190576
Total [baseline] (11.137 s) : 0, 11136522
Agent [candidate] (1.184 s) : 0, 1183604
Total [candidate] (11.006 s) : 0, 11005728
gantt
title petclinic - break down per module: candidate=1.61.0-SNAPSHOT~0d5f341a1a, baseline=1.61.0-SNAPSHOT~0ee26e896c
dateFormat X
axisFormat %s
section tracing
crashtracking [baseline] (1.216 ms) : 0, 1216
crashtracking [candidate] (1.209 ms) : 0, 1209
BytebuddyAgent [baseline] (636.88 ms) : 0, 636880
BytebuddyAgent [candidate] (636.314 ms) : 0, 636314
AgentMeter [baseline] (29.679 ms) : 0, 29679
AgentMeter [candidate] (29.676 ms) : 0, 29676
GlobalTracer [baseline] (259.839 ms) : 0, 259839
GlobalTracer [candidate] (255.68 ms) : 0, 255680
AppSec [baseline] (32.172 ms) : 0, 32172
AppSec [candidate] (32.138 ms) : 0, 32138
Debugger [baseline] (61.259 ms) : 0, 61259
Debugger [candidate] (60.452 ms) : 0, 60452
Remote Config [baseline] (620.48 µs) : 0, 620
Remote Config [candidate] (596.046 µs) : 0, 596
Telemetry [baseline] (8.184 ms) : 0, 8184
Telemetry [candidate] (8.019 ms) : 0, 8019
Flare Poller [baseline] (3.595 ms) : 0, 3595
Flare Poller [candidate] (6.471 ms) : 0, 6471
section appsec
crashtracking [baseline] (1.197 ms) : 0, 1197
crashtracking [candidate] (1.218 ms) : 0, 1218
BytebuddyAgent [baseline] (661.547 ms) : 0, 661547
BytebuddyAgent [candidate] (662.283 ms) : 0, 662283
AgentMeter [baseline] (12.21 ms) : 0, 12210
AgentMeter [candidate] (12.152 ms) : 0, 12152
GlobalTracer [baseline] (258.462 ms) : 0, 258462
GlobalTracer [candidate] (256.277 ms) : 0, 256277
IAST [baseline] (24.181 ms) : 0, 24181
IAST [candidate] (24.625 ms) : 0, 24625
AppSec [baseline] (177.962 ms) : 0, 177962
AppSec [candidate] (176.681 ms) : 0, 176681
Debugger [baseline] (66.786 ms) : 0, 66786
Debugger [candidate] (66.796 ms) : 0, 66796
Remote Config [baseline] (652.226 µs) : 0, 652
Remote Config [candidate] (593.837 µs) : 0, 594
Telemetry [baseline] (8.322 ms) : 0, 8322
Telemetry [candidate] (8.401 ms) : 0, 8401
Flare Poller [baseline] (3.59 ms) : 0, 3590
Flare Poller [candidate] (3.591 ms) : 0, 3591
section iast
crashtracking [baseline] (1.211 ms) : 0, 1211
crashtracking [candidate] (1.207 ms) : 0, 1207
BytebuddyAgent [baseline] (803.993 ms) : 0, 803993
BytebuddyAgent [candidate] (804.891 ms) : 0, 804891
AgentMeter [baseline] (11.507 ms) : 0, 11507
AgentMeter [candidate] (11.622 ms) : 0, 11622
GlobalTracer [baseline] (250.515 ms) : 0, 250515
GlobalTracer [candidate] (246.7 ms) : 0, 246700
IAST [baseline] (25.93 ms) : 0, 25930
IAST [candidate] (26.088 ms) : 0, 26088
AppSec [baseline] (27.18 ms) : 0, 27180
AppSec [candidate] (32.025 ms) : 0, 32025
Debugger [baseline] (71.043 ms) : 0, 71043
Debugger [candidate] (64.399 ms) : 0, 64399
Remote Config [baseline] (538.951 µs) : 0, 539
Remote Config [candidate] (532.56 µs) : 0, 533
Telemetry [baseline] (9.228 ms) : 0, 9228
Telemetry [candidate] (9.332 ms) : 0, 9332
Flare Poller [baseline] (3.442 ms) : 0, 3442
Flare Poller [candidate] (3.49 ms) : 0, 3490
section profiling
crashtracking [baseline] (1.196 ms) : 0, 1196
crashtracking [candidate] (1.176 ms) : 0, 1176
BytebuddyAgent [baseline] (685.558 ms) : 0, 685558
BytebuddyAgent [candidate] (685.549 ms) : 0, 685549
AgentMeter [baseline] (9.117 ms) : 0, 9117
AgentMeter [candidate] (8.999 ms) : 0, 8999
GlobalTracer [baseline] (217.13 ms) : 0, 217130
GlobalTracer [candidate] (212.454 ms) : 0, 212454
AppSec [baseline] (32.697 ms) : 0, 32697
AppSec [candidate] (32.709 ms) : 0, 32709
Debugger [baseline] (66.936 ms) : 0, 66936
Debugger [candidate] (65.247 ms) : 0, 65247
Remote Config [baseline] (584.336 µs) : 0, 584
Remote Config [candidate] (590.346 µs) : 0, 590
Telemetry [baseline] (7.875 ms) : 0, 7875
Telemetry [candidate] (8.51 ms) : 0, 8510
Flare Poller [baseline] (3.58 ms) : 0, 3580
Flare Poller [candidate] (3.513 ms) : 0, 3513
ProfilingAgent [baseline] (94.545 ms) : 0, 94545
ProfilingAgent [candidate] (93.672 ms) : 0, 93672
Profiling [baseline] (95.125 ms) : 0, 95125
Profiling [candidate] (94.224 ms) : 0, 94224
Startup time reports for insecure-bankgantt
title insecure-bank - global startup overhead: candidate=1.61.0-SNAPSHOT~0d5f341a1a, baseline=1.61.0-SNAPSHOT~0ee26e896c
dateFormat X
axisFormat %s
section tracing
Agent [baseline] (1.062 s) : 0, 1062103
Total [baseline] (8.893 s) : 0, 8893263
Agent [candidate] (1.075 s) : 0, 1075378
Total [candidate] (8.92 s) : 0, 8919850
section iast
Agent [baseline] (1.236 s) : 0, 1235623
Total [baseline] (9.593 s) : 0, 9593014
Agent [candidate] (1.23 s) : 0, 1230016
Total [candidate] (9.589 s) : 0, 9589437
gantt
title insecure-bank - break down per module: candidate=1.61.0-SNAPSHOT~0d5f341a1a, baseline=1.61.0-SNAPSHOT~0ee26e896c
dateFormat X
axisFormat %s
section tracing
crashtracking [baseline] (1.222 ms) : 0, 1222
crashtracking [candidate] (1.216 ms) : 0, 1216
BytebuddyAgent [baseline] (632.505 ms) : 0, 632505
BytebuddyAgent [candidate] (641.537 ms) : 0, 641537
AgentMeter [baseline] (29.423 ms) : 0, 29423
AgentMeter [candidate] (29.876 ms) : 0, 29876
GlobalTracer [baseline] (258.148 ms) : 0, 258148
GlobalTracer [candidate] (256.109 ms) : 0, 256109
AppSec [baseline] (31.833 ms) : 0, 31833
AppSec [candidate] (32.239 ms) : 0, 32239
Debugger [baseline] (59.78 ms) : 0, 59780
Debugger [candidate] (59.754 ms) : 0, 59754
Remote Config [baseline] (610.359 µs) : 0, 610
Remote Config [candidate] (582.782 µs) : 0, 583
Telemetry [baseline] (8.134 ms) : 0, 8134
Telemetry [candidate] (8.059 ms) : 0, 8059
Flare Poller [baseline] (4.25 ms) : 0, 4250
Flare Poller [candidate] (9.466 ms) : 0, 9466
section iast
crashtracking [baseline] (1.216 ms) : 0, 1216
crashtracking [candidate] (1.203 ms) : 0, 1203
BytebuddyAgent [baseline] (801.435 ms) : 0, 801435
BytebuddyAgent [candidate] (800.888 ms) : 0, 800888
AgentMeter [baseline] (11.511 ms) : 0, 11511
AgentMeter [candidate] (11.421 ms) : 0, 11421
GlobalTracer [baseline] (249.588 ms) : 0, 249588
GlobalTracer [candidate] (244.788 ms) : 0, 244788
AppSec [baseline] (26.916 ms) : 0, 26916
AppSec [candidate] (32.84 ms) : 0, 32840
Debugger [baseline] (69.819 ms) : 0, 69819
Debugger [candidate] (61.347 ms) : 0, 61347
Remote Config [baseline] (533.616 µs) : 0, 534
Remote Config [candidate] (538.431 µs) : 0, 538
Telemetry [baseline] (9.234 ms) : 0, 9234
Telemetry [candidate] (11.084 ms) : 0, 11084
Flare Poller [baseline] (3.45 ms) : 0, 3450
Flare Poller [candidate] (3.829 ms) : 0, 3829
IAST [baseline] (25.703 ms) : 0, 25703
IAST [candidate] (25.922 ms) : 0, 25922
LoadParameters
See matching parameters
SummaryFound 2 performance improvements and 2 performance regressions! Performance is the same for 16 metrics, 16 unstable metrics.
Request duration reports for petclinicgantt
title petclinic - request duration [CI 0.99] : candidate=1.61.0-SNAPSHOT~0d5f341a1a, baseline=1.61.0-SNAPSHOT~0ee26e896c
dateFormat X
axisFormat %s
section baseline
no_agent (18.417 ms) : 18230, 18603
. : milestone, 18417,
appsec (18.658 ms) : 18469, 18848
. : milestone, 18658,
code_origins (17.81 ms) : 17636, 17984
. : milestone, 17810,
iast (18.3 ms) : 18117, 18483
. : milestone, 18300,
profiling (19.546 ms) : 19347, 19746
. : milestone, 19546,
tracing (17.733 ms) : 17558, 17908
. : milestone, 17733,
section candidate
no_agent (18.486 ms) : 18305, 18668
. : milestone, 18486,
appsec (19.167 ms) : 18970, 19364
. : milestone, 19167,
code_origins (18.677 ms) : 18490, 18864
. : milestone, 18677,
iast (18.053 ms) : 17874, 18232
. : milestone, 18053,
profiling (19.014 ms) : 18826, 19202
. : milestone, 19014,
tracing (17.842 ms) : 17663, 18021
. : milestone, 17842,
Request duration reports for insecure-bankgantt
title insecure-bank - request duration [CI 0.99] : candidate=1.61.0-SNAPSHOT~0d5f341a1a, baseline=1.61.0-SNAPSHOT~0ee26e896c
dateFormat X
axisFormat %s
section baseline
no_agent (1.227 ms) : 1215, 1239
. : milestone, 1227,
iast (3.284 ms) : 3238, 3330
. : milestone, 3284,
iast_FULL (6.059 ms) : 5997, 6121
. : milestone, 6059,
iast_GLOBAL (3.631 ms) : 3578, 3683
. : milestone, 3631,
profiling (2.195 ms) : 2174, 2216
. : milestone, 2195,
tracing (1.85 ms) : 1835, 1865
. : milestone, 1850,
section candidate
no_agent (1.225 ms) : 1214, 1237
. : milestone, 1225,
iast (3.389 ms) : 3340, 3438
. : milestone, 3389,
iast_FULL (5.82 ms) : 5762, 5879
. : milestone, 5820,
iast_GLOBAL (3.632 ms) : 3574, 3690
. : milestone, 3632,
profiling (2.143 ms) : 2123, 2163
. : milestone, 2143,
tracing (1.852 ms) : 1836, 1867
. : milestone, 1852,
DacapoParameters
See matching parameters
SummaryFound 0 performance improvements and 0 performance regressions! Performance is the same for 12 metrics, 0 unstable metrics. Execution time for tomcatgantt
title tomcat - execution time [CI 0.99] : candidate=1.61.0-SNAPSHOT~0d5f341a1a, baseline=1.61.0-SNAPSHOT~0ee26e896c
dateFormat X
axisFormat %s
section baseline
no_agent (1.489 ms) : 1477, 1501
. : milestone, 1489,
appsec (2.543 ms) : 2488, 2598
. : milestone, 2543,
iast (2.274 ms) : 2205, 2342
. : milestone, 2274,
iast_GLOBAL (2.315 ms) : 2245, 2384
. : milestone, 2315,
profiling (2.103 ms) : 2048, 2157
. : milestone, 2103,
tracing (2.081 ms) : 2028, 2134
. : milestone, 2081,
section candidate
no_agent (1.487 ms) : 1476, 1499
. : milestone, 1487,
appsec (2.534 ms) : 2479, 2588
. : milestone, 2534,
iast (2.26 ms) : 2191, 2328
. : milestone, 2260,
iast_GLOBAL (2.314 ms) : 2245, 2383
. : milestone, 2314,
profiling (2.092 ms) : 2038, 2146
. : milestone, 2092,
tracing (2.071 ms) : 2018, 2124
. : milestone, 2071,
Execution time for biojavagantt
title biojava - execution time [CI 0.99] : candidate=1.61.0-SNAPSHOT~0d5f341a1a, baseline=1.61.0-SNAPSHOT~0ee26e896c
dateFormat X
axisFormat %s
section baseline
no_agent (15.543 s) : 15543000, 15543000
. : milestone, 15543000,
appsec (14.758 s) : 14758000, 14758000
. : milestone, 14758000,
iast (18.379 s) : 18379000, 18379000
. : milestone, 18379000,
iast_GLOBAL (17.838 s) : 17838000, 17838000
. : milestone, 17838000,
profiling (14.892 s) : 14892000, 14892000
. : milestone, 14892000,
tracing (15.016 s) : 15016000, 15016000
. : milestone, 15016000,
section candidate
no_agent (15.125 s) : 15125000, 15125000
. : milestone, 15125000,
appsec (14.548 s) : 14548000, 14548000
. : milestone, 14548000,
iast (18.753 s) : 18753000, 18753000
. : milestone, 18753000,
iast_GLOBAL (17.752 s) : 17752000, 17752000
. : milestone, 17752000,
profiling (14.882 s) : 14882000, 14882000
. : milestone, 14882000,
tracing (15.091 s) : 15091000, 15091000
. : milestone, 15091000,
|
…ataDog/dd-trace-java into dougqh/trace-interceptors-optimization
| } | ||
|
|
||
| @Test | ||
| public void noInterceptorsTest() { |
There was a problem hiding this comment.
❔ question: Is there a reason to get rid of this safety check?
There was a problem hiding this comment.
I accidentally created the same test twice in my previous PR, so I removed one of them.
| * Implements a copy on write array list where interceptors are added to the list ordered by | ||
| * TraceInterceptor priority | ||
| */ | ||
| static final class TraceInterceptors { |
There was a problem hiding this comment.
🎯 suggestion: What about moving it in its own file?
There was a problem hiding this comment.
I didn't do that because it is currently tightly coupled with CoreTracer.
Normally, I'd have TraceInterceptors be a composite and implement TraceInterceptor and just generally have better encapsulation. However, the interaction between CoreTracer, TraceInterceptors, and updating the SpanList didn't lend itself to that.
|
Local spring petclinic throughput tests, show a 16% reduction in throughput penalty with ample heap, unchanged throughput with low heap, and slightly reduced throughput with moderately low heap |
|
/merge |
|
View all feedbacks in Devflow UI.
The expected merge time in
This PR is rejected because it was updated |
…ataDog/dd-trace-java into dougqh/trace-interceptors-optimization
|
/merge |
|
View all feedbacks in Devflow UI.
The expected merge time in
mergequeue build completed successfully, but the github api returned an error while merging the pr. DetailsError: PUT https://api.github.com/repos/DataDog/dd-trace-java/pulls/11002/merge: 405 Merge already in progress [] (Request ID: E510:16DE77:2EEF0CA:C3B51B6:69CC4905) FullStacktrace: |
|
/merge |
|
View all feedbacks in Devflow UI.
The expected merge time in
|
6c8fccc
into
master
What Does This Do
Replaces the interceptors ConcurrentSkipListSet with a priority ordered copy on write "list"
Motivation
The ConcurrentSkipListSet iteration was sometimes adding 7 GiB (out of 250 GiB) in a span creation stress test. Probably from escape analysis sporadically failing based on inlining decisions.
Since adding to the list is rare, ConcurrentSkipListSet isn't really necessary for this particular problem. Maintaining a sorted list through insertion sort accomplishes the same thing without the risk of high allocation.
Additional Notes
Escape analysis doesn't always fail, so sometimes this change is allocation neutral. However even in the case where analysis succeeds, this change provides a small benefit from better cache locality
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.