-
Notifications
You must be signed in to change notification settings - Fork 320
Add support for Java 25 Structured Concurrency API #9276
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.
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
Conversation
BenchmarksStartupParameters
See matching parameters
SummaryFound 6 performance improvements and 0 performance regressions! Performance is the same for 40 metrics, 13 unstable metrics.
Startup time reports for insecure-bankgantt
title insecure-bank - global startup overhead: candidate=1.54.0-SNAPSHOT~5d0c1d06bb, baseline=1.54.0-SNAPSHOT~81cdab0f050
dateFormat X
axisFormat %s
section tracing
Agent [baseline] (1.071 s) : 0, 1071000
Total [baseline] (8.675 s) : 0, 8674806
Agent [candidate] (1.047 s) : 0, 1047443
Total [candidate] (8.625 s) : 0, 8625424
section iast
Agent [baseline] (1.194 s) : 0, 1193886
Total [baseline] (9.315 s) : 0, 9314776
Agent [candidate] (1.19 s) : 0, 1190113
Total [candidate] (9.369 s) : 0, 9369074
gantt
title insecure-bank - break down per module: candidate=1.54.0-SNAPSHOT~5d0c1d06bb, baseline=1.54.0-SNAPSHOT~81cdab0f050
dateFormat X
axisFormat %s
section tracing
crashtracking [baseline] (1.465 ms) : 0, 1465
crashtracking [candidate] (1.459 ms) : 0, 1459
BytebuddyAgent [baseline] (739.133 ms) : 0, 739133
BytebuddyAgent [candidate] (734.099 ms) : 0, 734099
GlobalTracer [baseline] (254.581 ms) : 0, 254581
GlobalTracer [candidate] (242.65 ms) : 0, 242650
AppSec [baseline] (30.886 ms) : 0, 30886
AppSec [candidate] (30.162 ms) : 0, 30162
Debugger [baseline] (6.465 ms) : 0, 6465
Debugger [candidate] (6.427 ms) : 0, 6427
Remote Config [baseline] (700.984 µs) : 0, 701
Remote Config [candidate] (693.787 µs) : 0, 694
Telemetry [baseline] (16.526 ms) : 0, 16526
Telemetry [candidate] (10.932 ms) : 0, 10932
section iast
crashtracking [baseline] (1.47 ms) : 0, 1470
crashtracking [candidate] (1.461 ms) : 0, 1461
BytebuddyAgent [baseline] (856.067 ms) : 0, 856067
BytebuddyAgent [candidate] (859.267 ms) : 0, 859267
GlobalTracer [baseline] (247.054 ms) : 0, 247054
GlobalTracer [candidate] (234.872 ms) : 0, 234872
AppSec [baseline] (24.631 ms) : 0, 24631
AppSec [candidate] (25.462 ms) : 0, 25462
Debugger [baseline] (6.044 ms) : 0, 6044
Debugger [candidate] (7.808 ms) : 0, 7808
Remote Config [baseline] (608.134 µs) : 0, 608
Remote Config [candidate] (609.766 µs) : 0, 610
Telemetry [baseline] (8.144 ms) : 0, 8144
Telemetry [candidate] (8.264 ms) : 0, 8264
IAST [baseline] (28.857 ms) : 0, 28857
IAST [candidate] (31.273 ms) : 0, 31273
Startup time reports for petclinicgantt
title petclinic - global startup overhead: candidate=1.54.0-SNAPSHOT~5d0c1d06bb, baseline=1.54.0-SNAPSHOT~81cdab0f050
dateFormat X
axisFormat %s
section tracing
Agent [baseline] (1.07 s) : 0, 1070444
Total [baseline] (10.759 s) : 0, 10758671
Agent [candidate] (1.046 s) : 0, 1046254
Total [candidate] (10.782 s) : 0, 10781535
section appsec
Agent [baseline] (1.237 s) : 0, 1236703
Total [baseline] (10.937 s) : 0, 10937043
Agent [candidate] (1.223 s) : 0, 1222551
Total [candidate] (10.803 s) : 0, 10803194
section iast
Agent [baseline] (1.19 s) : 0, 1189757
Total [baseline] (11.074 s) : 0, 11073793
Agent [candidate] (1.18 s) : 0, 1179790
Total [candidate] (10.909 s) : 0, 10908938
section profiling
Agent [baseline] (1.208 s) : 0, 1208326
Total [baseline] (10.926 s) : 0, 10925823
Agent [candidate] (1.2 s) : 0, 1200153
Total [candidate] (11.015 s) : 0, 11015028
gantt
title petclinic - break down per module: candidate=1.54.0-SNAPSHOT~5d0c1d06bb, baseline=1.54.0-SNAPSHOT~81cdab0f050
dateFormat X
axisFormat %s
section tracing
crashtracking [baseline] (1.477 ms) : 0, 1477
crashtracking [candidate] (1.449 ms) : 0, 1449
BytebuddyAgent [baseline] (738.881 ms) : 0, 738881
BytebuddyAgent [candidate] (732.269 ms) : 0, 732269
GlobalTracer [baseline] (254.239 ms) : 0, 254239
GlobalTracer [candidate] (242.499 ms) : 0, 242499
AppSec [baseline] (30.885 ms) : 0, 30885
AppSec [candidate] (30.082 ms) : 0, 30082
Debugger [baseline] (6.461 ms) : 0, 6461
Debugger [candidate] (6.461 ms) : 0, 6461
Remote Config [baseline] (707.782 µs) : 0, 708
Remote Config [candidate] (687.404 µs) : 0, 687
Telemetry [baseline] (16.422 ms) : 0, 16422
Telemetry [candidate] (11.697 ms) : 0, 11697
section appsec
crashtracking [baseline] (1.46 ms) : 0, 1460
crashtracking [candidate] (1.459 ms) : 0, 1459
BytebuddyAgent [baseline] (757.004 ms) : 0, 757004
BytebuddyAgent [candidate] (754.417 ms) : 0, 754417
GlobalTracer [baseline] (246.294 ms) : 0, 246294
GlobalTracer [candidate] (235.447 ms) : 0, 235447
AppSec [baseline] (171.841 ms) : 0, 171841
AppSec [candidate] (168.472 ms) : 0, 168472
Debugger [baseline] (6.046 ms) : 0, 6046
Debugger [candidate] (7.569 ms) : 0, 7569
Remote Config [baseline] (625.58 µs) : 0, 626
Remote Config [candidate] (659.349 µs) : 0, 659
Telemetry [baseline] (8.375 ms) : 0, 8375
Telemetry [candidate] (10.018 ms) : 0, 10018
IAST [baseline] (23.761 ms) : 0, 23761
IAST [candidate] (23.43 ms) : 0, 23430
section iast
crashtracking [baseline] (1.455 ms) : 0, 1455
crashtracking [candidate] (1.479 ms) : 0, 1479
BytebuddyAgent [baseline] (851.613 ms) : 0, 851613
BytebuddyAgent [candidate] (851.009 ms) : 0, 851009
GlobalTracer [baseline] (246.483 ms) : 0, 246483
GlobalTracer [candidate] (233.294 ms) : 0, 233294
AppSec [baseline] (24.622 ms) : 0, 24622
AppSec [candidate] (26.822 ms) : 0, 26822
Debugger [baseline] (6.025 ms) : 0, 6025
Debugger [candidate] (7.007 ms) : 0, 7007
Remote Config [baseline] (589.426 µs) : 0, 589
Remote Config [candidate] (599.274 µs) : 0, 599
Telemetry [baseline] (8.099 ms) : 0, 8099
Telemetry [candidate] (8.298 ms) : 0, 8298
IAST [baseline] (29.77 ms) : 0, 29770
IAST [candidate] (30.208 ms) : 0, 30208
section profiling
ProfilingAgent [baseline] (107.782 ms) : 0, 107782
ProfilingAgent [candidate] (108.168 ms) : 0, 108168
crashtracking [baseline] (1.425 ms) : 0, 1425
crashtracking [candidate] (1.431 ms) : 0, 1431
BytebuddyAgent [baseline] (760.934 ms) : 0, 760934
BytebuddyAgent [candidate] (762.752 ms) : 0, 762752
GlobalTracer [baseline] (233.086 ms) : 0, 233086
GlobalTracer [candidate] (223.018 ms) : 0, 223018
AppSec [baseline] (31.046 ms) : 0, 31046
AppSec [candidate] (30.578 ms) : 0, 30578
Debugger [baseline] (7.384 ms) : 0, 7384
Debugger [candidate] (6.759 ms) : 0, 6759
Remote Config [baseline] (702.457 µs) : 0, 702
Remote Config [candidate] (680.341 µs) : 0, 680
Telemetry [baseline] (14.848 ms) : 0, 14848
Telemetry [candidate] (16.281 ms) : 0, 16281
Profiling [baseline] (108.431 ms) : 0, 108431
Profiling [candidate] (108.917 ms) : 0, 108917
LoadParameters
See matching parameters
SummaryFound 1 performance improvements and 2 performance regressions! Performance is the same for 9 metrics, 12 unstable metrics.
Request duration reports for insecure-bankgantt
title insecure-bank - request duration [CI 0.99] : candidate=1.54.0-SNAPSHOT~5d0c1d06bb, baseline=1.54.0-SNAPSHOT~81cdab0f050
dateFormat X
axisFormat %s
section baseline
no_agent (4.405 ms) : 4356, 4455
. : milestone, 4405,
iast (9.566 ms) : 9392, 9739
. : milestone, 9566,
iast_FULL (13.81 ms) : 13536, 14084
. : milestone, 13810,
iast_GLOBAL (10.774 ms) : 10580, 10968
. : milestone, 10774,
profiling (8.907 ms) : 8768, 9046
. : milestone, 8907,
tracing (7.863 ms) : 7742, 7983
. : milestone, 7863,
section candidate
no_agent (4.364 ms) : 4314, 4413
. : milestone, 4364,
iast (9.656 ms) : 9493, 9820
. : milestone, 9656,
iast_FULL (14.235 ms) : 13948, 14522
. : milestone, 14235,
iast_GLOBAL (10.283 ms) : 10102, 10464
. : milestone, 10283,
profiling (8.776 ms) : 8640, 8912
. : milestone, 8776,
tracing (7.921 ms) : 7805, 8038
. : milestone, 7921,
Request duration reports for petclinicgantt
title petclinic - request duration [CI 0.99] : candidate=1.54.0-SNAPSHOT~5d0c1d06bb, baseline=1.54.0-SNAPSHOT~81cdab0f050
dateFormat X
axisFormat %s
section baseline
no_agent (37.856 ms) : 37545, 38167
. : milestone, 37856,
appsec (46.738 ms) : 46339, 47137
. : milestone, 46738,
code_origins (44.491 ms) : 44111, 44871
. : milestone, 44491,
iast (44.855 ms) : 44463, 45247
. : milestone, 44855,
profiling (46.62 ms) : 46201, 47039
. : milestone, 46620,
tracing (44.956 ms) : 44570, 45341
. : milestone, 44956,
section candidate
no_agent (37.773 ms) : 37462, 38084
. : milestone, 37773,
appsec (49.417 ms) : 48983, 49851
. : milestone, 49417,
code_origins (44.1 ms) : 43707, 44492
. : milestone, 44100,
iast (45.885 ms) : 45492, 46278
. : milestone, 45885,
profiling (49.657 ms) : 49177, 50136
. : milestone, 49657,
tracing (44.193 ms) : 43812, 44575
. : milestone, 44193,
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 biojavagantt
title biojava - execution time [CI 0.99] : candidate=1.54.0-SNAPSHOT~5d0c1d06bb, baseline=1.54.0-SNAPSHOT~81cdab0f050
dateFormat X
axisFormat %s
section baseline
no_agent (14.78 s) : 14780000, 14780000
. : milestone, 14780000,
appsec (14.739 s) : 14739000, 14739000
. : milestone, 14739000,
iast (18.508 s) : 18508000, 18508000
. : milestone, 18508000,
iast_GLOBAL (18.131 s) : 18131000, 18131000
. : milestone, 18131000,
profiling (15.478 s) : 15478000, 15478000
. : milestone, 15478000,
tracing (15.07 s) : 15070000, 15070000
. : milestone, 15070000,
section candidate
no_agent (15.573 s) : 15573000, 15573000
. : milestone, 15573000,
appsec (14.73 s) : 14730000, 14730000
. : milestone, 14730000,
iast (18.405 s) : 18405000, 18405000
. : milestone, 18405000,
iast_GLOBAL (17.943 s) : 17943000, 17943000
. : milestone, 17943000,
profiling (15.465 s) : 15465000, 15465000
. : milestone, 15465000,
tracing (14.809 s) : 14809000, 14809000
. : milestone, 14809000,
Execution time for tomcatgantt
title tomcat - execution time [CI 0.99] : candidate=1.54.0-SNAPSHOT~5d0c1d06bb, baseline=1.54.0-SNAPSHOT~81cdab0f050
dateFormat X
axisFormat %s
section baseline
no_agent (1.472 ms) : 1460, 1483
. : milestone, 1472,
appsec (3.687 ms) : 3471, 3903
. : milestone, 3687,
iast (2.187 ms) : 2125, 2250
. : milestone, 2187,
iast_GLOBAL (2.228 ms) : 2165, 2291
. : milestone, 2228,
profiling (2.047 ms) : 1995, 2098
. : milestone, 2047,
tracing (2.019 ms) : 1969, 2068
. : milestone, 2019,
section candidate
no_agent (1.474 ms) : 1463, 1486
. : milestone, 1474,
appsec (3.628 ms) : 3412, 3845
. : milestone, 3628,
iast (2.191 ms) : 2129, 2254
. : milestone, 2191,
iast_GLOBAL (2.234 ms) : 2171, 2297
. : milestone, 2234,
profiling (2.048 ms) : 1996, 2100
. : milestone, 2048,
tracing (2.011 ms) : 1962, 2059
. : milestone, 2011,
|
AlexeyKuznetsov-DD
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM in general, left couple of minor comments.
| ext { | ||
| minJavaVersionForTests = JavaVersion.VERSION_25 | ||
| } | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion: how about to use Kotlin DSL for new build.gradle?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would rather wait for having a clear migration path rather than trying to build new conventions and plugins as part of this PR. WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think that using kotlin DSL is gonna change that or not. However it clearly help IDE support now. In my opinion I would try to use kotlin DSL regardless. This can also help exercise our skills in that regard.
That said while I would prefer kotlin dsl usage regardless, I won't push for it either at this time.
...nstrumentation/java/concurrent/structuredconcurrency/StructuredTaskScopeInstrumentation.java
Outdated
Show resolved
Hide resolved
| org.gradle.java.installations.auto-download=false | ||
| # 8 and 11 is needed to build | ||
| org.gradle.java.installations.fromEnv=JAVA_8_HOME,JAVA_11_HOME,JAVA_17_HOME,JAVA_21_HOME | ||
| org.gradle.java.installations.fromEnv=JAVA_8_HOME,JAVA_11_HOME,JAVA_17_HOME,JAVA_21_HOME,JAVA_25_HOME |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BTW, should we have JAVA_24_HOME too?
cc: @sarahchen6
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm 🤔 Since we don't build anything in Java 24 (only test), I don't think we need it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like finding JAVA_24_HOME here for tests is sufficient: https://github.com/DataDog/dd-trace-java/blob/master/gradle/java_no_deps.gradle#L151-L191
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It feels like we should move to some other mechanism to detect JVMs...
Gradle seems to have modern provider and discovery mechanism.
It could be interesting to have a look as part of a broader refactoring cc @bric3
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like finding JAVA_24_HOME here for tests is sufficient
It should be for -PtestJvm=stable yes 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sarahchen6 I recall that we have idea to rename stable to something like latest-non-lts?
WDYT on renaming?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I remember we didn't go through with it because of the idea that the build image should always provide the latest "stable" version in addition to specific LTS versions that we want to test. It is on the CI, not the available build images, to avoid duplicate testing (i.e. we do not test stable when stable == LTS). There is a conversation on this PR for more context: DataDog/dd-trace-java-docker-build#107
6b93ebd to
603b585
Compare
| ext { | ||
| minJavaVersionForTests = JavaVersion.VERSION_25 | ||
| } | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think that using kotlin DSL is gonna change that or not. However it clearly help IDE support now. In my opinion I would try to use kotlin DSL regardless. This can also help exercise our skills in that regard.
That said while I would prefer kotlin dsl usage regardless, I won't push for it either at this time.
| // Set all compile tasks to use JDK21 but let instrumentation code targets 1.8 compatibility | ||
| project.tasks.withType(AbstractCompile).configureEach { | ||
| setJavaVersion(it, 25) | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion:
| // Set all compile tasks to use JDK21 but let instrumentation code targets 1.8 compatibility | |
| project.tasks.withType(AbstractCompile).configureEach { | |
| setJavaVersion(it, 25) | |
| } | |
| // Sets all compile tasks to use JDK25 but ensures instrumentation code still targets 1.8 compatibility | |
| project.tasks.withType(AbstractCompile).configureEach { | |
| setJavaVersion(it, 25) | |
| } |
| } | ||
|
|
||
| dependencies { | ||
| testImplementation project(':dd-java-agent:instrumentation:trace-annotation') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: FYI, Groovy 5 rc1 is out, https://groovy-lang.org/changelogs/changelog-5.0.0-rc-1.html
But requires a JDK 11 to run.
603b585 to
f408223
Compare
da96528 to
da9c278
Compare
|
🎯 Code Coverage 🔗 Commit SHA: 5d0c1d0 | Docs | Was this helpful? Give us feedback! |
Add instrumentation alias Limit activation to Java < 25 Fix context store declaration and usage to match RunnableInstrumentation Remove unused generic
9c0d9bb to
d24dfed
Compare
|
Re-requesting reviews as most of the content changed from the original draft. |
...umentation/java/concurrent/structuredconcurrency21/StructuredTaskScope21Instrumentation.java
Show resolved
Hide resolved
| import net.bytebuddy.asm.Advice.This; | ||
|
|
||
| // WARNING: | ||
| // This instrumentation is tested using smoke tests as instrumented tests cannot run using Java 25. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add description of WHY it cannot run?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sadly I did not keep track of all the failures.
The first immediate one that come to mind would be that our Groovy version won't run using Java 25 but there are more reasons. Would it make to document them as our goal next quarter will be to get rid of all of this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added the groovy / spock rationale to the comment then 👍
| // Disable plugin tasks that do not support Java 25: | ||
| // * forbiddenApis is missing classes | ||
| // * spotless as the google-java-format version does not support Java 25 and can't be changed once applied | ||
| // * spotbugs failed to read class using newer bytecode versions | ||
| forbiddenApisMain { | ||
| failOnMissingClasses = false | ||
| } | ||
| ['spotlessApply', 'spotlessCheck', 'spotlessJava', 'spotbugsMain'].each { | ||
| tasks.named(it).configure { enabled = false } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bric3 if we decouple Gradle JDK this should be fixed?
@PerfectSlayer Maybe we should try to use forbiddenApi v3.9?
I can see this PR Update ASM to version 9.8 for support of Java 25 bytecode; add signatures and build support for Java 24 GA
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, you can try to update it and I can test it there to see if I can active it in this module.
For info, the :java-21 module is also disabling those plugins as not working with 21 either 😞
d24dfed to
5d0c1d0
Compare
What Does This Do
This PR brings support for the Structured Concurrency API on JDK 25 related to the JEP 505.
Motivation
Structured Concurrency is still a preview API but is leveraged internally by frameworks and libraries.
In order to not break context tracking, we keep supporting it the preview feature.
Additional Notes
About testing, as there is no current way to run our test framework using Java 25, the related instrumentation tests are done using smoke tests. And as smoke tests don't run either on Java 25, they will run on smoke test job for Java 8, with an hardcoded fork to a JVM 25 to run the tested application.
I would gladly rewriting the test suite as instrumented test as soon as it becomes possible as it will be misleading that Java 8 smoke tests test Java 25 preview feature.
Contributor Checklist
type:and (comp:orinst:) labels in addition to any usefull labelsclose,fixor any linking keywords when referencing an issue.Use
solvesinstead, and assign the PR milestone to the issueJira ticket: LANGPLAT-702