-
Notifications
You must be signed in to change notification settings - Fork 311
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
base: master
Are you sure you want to change the base?
Conversation
BenchmarksStartupParameters
See matching parameters
SummaryFound 0 performance improvements and 0 performance regressions! Performance is the same for 46 metrics, 13 unstable metrics. Startup time reports for insecure-bankgantt
title insecure-bank - global startup overhead: candidate=1.51.1-SNAPSHOT~603b5851ac, baseline=1.51.1-SNAPSHOT~3a35f77b25
dateFormat X
axisFormat %s
section tracing
Agent [baseline] (1.047 s) : 0, 1046527
Total [baseline] (8.595 s) : 0, 8595015
Agent [candidate] (1.048 s) : 0, 1048470
Total [candidate] (8.616 s) : 0, 8616153
section iast
Agent [baseline] (1.184 s) : 0, 1184139
Total [baseline] (9.328 s) : 0, 9328057
Agent [candidate] (1.181 s) : 0, 1180812
Total [candidate] (9.298 s) : 0, 9297607
gantt
title insecure-bank - break down per module: candidate=1.51.1-SNAPSHOT~603b5851ac, baseline=1.51.1-SNAPSHOT~3a35f77b25
dateFormat X
axisFormat %s
section tracing
crashtracking [baseline] (1.436 ms) : 0, 1436
crashtracking [candidate] (1.435 ms) : 0, 1435
BytebuddyAgent [baseline] (732.583 ms) : 0, 732583
BytebuddyAgent [candidate] (734.631 ms) : 0, 734631
GlobalTracer [baseline] (241.877 ms) : 0, 241877
GlobalTracer [candidate] (242.738 ms) : 0, 242738
AppSec [baseline] (30.19 ms) : 0, 30190
AppSec [candidate] (30.058 ms) : 0, 30058
Debugger [baseline] (6.081 ms) : 0, 6081
Debugger [candidate] (6.062 ms) : 0, 6062
Remote Config [baseline] (655.148 µs) : 0, 655
Remote Config [candidate] (647.215 µs) : 0, 647
Telemetry [baseline] (12.675 ms) : 0, 12675
Telemetry [candidate] (11.851 ms) : 0, 11851
section iast
crashtracking [baseline] (1.437 ms) : 0, 1437
crashtracking [candidate] (1.436 ms) : 0, 1436
BytebuddyAgent [baseline] (856.05 ms) : 0, 856050
BytebuddyAgent [candidate] (853.587 ms) : 0, 853587
GlobalTracer [baseline] (232.565 ms) : 0, 232565
GlobalTracer [candidate] (232.153 ms) : 0, 232153
IAST [baseline] (26.99 ms) : 0, 26990
IAST [candidate] (30.097 ms) : 0, 30097
AppSec [baseline] (29.694 ms) : 0, 29694
AppSec [candidate] (26.24 ms) : 0, 26240
Debugger [baseline] (7.607 ms) : 0, 7607
Debugger [candidate] (7.617 ms) : 0, 7617
Remote Config [baseline] (601.346 µs) : 0, 601
Remote Config [candidate] (614.325 µs) : 0, 614
Telemetry [baseline] (8.189 ms) : 0, 8189
Telemetry [candidate] (8.103 ms) : 0, 8103
Startup time reports for petclinicgantt
title petclinic - global startup overhead: candidate=1.51.1-SNAPSHOT~603b5851ac, baseline=1.51.1-SNAPSHOT~3a35f77b25
dateFormat X
axisFormat %s
section tracing
Agent [baseline] (1.042 s) : 0, 1042339
Total [baseline] (10.757 s) : 0, 10757476
Agent [candidate] (1.057 s) : 0, 1056772
Total [candidate] (10.762 s) : 0, 10762161
section appsec
Agent [baseline] (1.223 s) : 0, 1223278
Total [baseline] (10.765 s) : 0, 10765403
Agent [candidate] (1.227 s) : 0, 1227179
Total [candidate] (10.896 s) : 0, 10895758
section iast
Agent [baseline] (1.177 s) : 0, 1176953
Total [baseline] (10.862 s) : 0, 10861825
Agent [candidate] (1.182 s) : 0, 1181519
Total [candidate] (10.94 s) : 0, 10940392
section profiling
Agent [baseline] (1.192 s) : 0, 1192493
Total [baseline] (10.883 s) : 0, 10883195
Agent [candidate] (1.203 s) : 0, 1202876
Total [candidate] (10.895 s) : 0, 10895308
gantt
title petclinic - break down per module: candidate=1.51.1-SNAPSHOT~603b5851ac, baseline=1.51.1-SNAPSHOT~3a35f77b25
dateFormat X
axisFormat %s
section tracing
crashtracking [baseline] (1.432 ms) : 0, 1432
crashtracking [candidate] (1.45 ms) : 0, 1450
BytebuddyAgent [baseline] (731.494 ms) : 0, 731494
BytebuddyAgent [candidate] (740.308 ms) : 0, 740308
GlobalTracer [baseline] (241.228 ms) : 0, 241228
GlobalTracer [candidate] (244.306 ms) : 0, 244306
AppSec [baseline] (30.04 ms) : 0, 30040
AppSec [candidate] (30.48 ms) : 0, 30480
Debugger [baseline] (6.028 ms) : 0, 6028
Debugger [candidate] (6.066 ms) : 0, 6066
Remote Config [baseline] (640.156 µs) : 0, 640
Remote Config [candidate] (659.761 µs) : 0, 660
Telemetry [baseline] (10.542 ms) : 0, 10542
Telemetry [candidate] (12.316 ms) : 0, 12316
section appsec
crashtracking [baseline] (1.434 ms) : 0, 1434
crashtracking [candidate] (1.438 ms) : 0, 1438
BytebuddyAgent [baseline] (755.66 ms) : 0, 755660
BytebuddyAgent [candidate] (757.526 ms) : 0, 757526
GlobalTracer [baseline] (235.465 ms) : 0, 235465
GlobalTracer [candidate] (236.667 ms) : 0, 236667
IAST [baseline] (23.59 ms) : 0, 23590
IAST [candidate] (23.767 ms) : 0, 23767
AppSec [baseline] (169.217 ms) : 0, 169217
AppSec [candidate] (170.301 ms) : 0, 170301
Debugger [baseline] (7.2 ms) : 0, 7200
Debugger [candidate] (6.527 ms) : 0, 6527
Remote Config [baseline] (617.645 µs) : 0, 618
Remote Config [candidate] (622.084 µs) : 0, 622
Telemetry [baseline] (8.968 ms) : 0, 8968
Telemetry [candidate] (9.202 ms) : 0, 9202
section iast
crashtracking [baseline] (1.434 ms) : 0, 1434
crashtracking [candidate] (1.435 ms) : 0, 1435
BytebuddyAgent [baseline] (850.268 ms) : 0, 850268
BytebuddyAgent [candidate] (852.155 ms) : 0, 852155
GlobalTracer [baseline] (231.571 ms) : 0, 231571
GlobalTracer [candidate] (233.633 ms) : 0, 233633
IAST [baseline] (30.088 ms) : 0, 30088
IAST [candidate] (29.479 ms) : 0, 29479
AppSec [baseline] (27.083 ms) : 0, 27083
AppSec [candidate] (26.672 ms) : 0, 26672
Debugger [baseline] (6.757 ms) : 0, 6757
Debugger [candidate] (8.373 ms) : 0, 8373
Remote Config [baseline] (604.381 µs) : 0, 604
Remote Config [candidate] (585.257 µs) : 0, 585
Telemetry [baseline] (8.099 ms) : 0, 8099
Telemetry [candidate] (8.159 ms) : 0, 8159
section profiling
crashtracking [baseline] (1.402 ms) : 0, 1402
crashtracking [candidate] (1.419 ms) : 0, 1419
BytebuddyAgent [baseline] (759.357 ms) : 0, 759357
BytebuddyAgent [candidate] (766.52 ms) : 0, 766520
GlobalTracer [baseline] (221.771 ms) : 0, 221771
GlobalTracer [candidate] (222.831 ms) : 0, 222831
AppSec [baseline] (29.852 ms) : 0, 29852
AppSec [candidate] (30.323 ms) : 0, 30323
Debugger [baseline] (6.269 ms) : 0, 6269
Debugger [candidate] (6.315 ms) : 0, 6315
Remote Config [baseline] (702.616 µs) : 0, 703
Remote Config [candidate] (707.415 µs) : 0, 707
Telemetry [baseline] (15.159 ms) : 0, 15159
Telemetry [candidate] (16.303 ms) : 0, 16303
ProfilingAgent [baseline] (108.656 ms) : 0, 108656
ProfilingAgent [candidate] (108.782 ms) : 0, 108782
Profiling [baseline] (109.296 ms) : 0, 109296
Profiling [candidate] (109.429 ms) : 0, 109429
LoadParameters
See matching parameters
SummaryFound 3 performance improvements and 1 performance regressions! Performance is the same for 8 metrics, 12 unstable metrics.
Request duration reports for petclinicgantt
title petclinic - request duration [CI 0.99] : candidate=1.51.1-SNAPSHOT~603b5851ac, baseline=1.51.1-SNAPSHOT~3a35f77b25
dateFormat X
axisFormat %s
section baseline
no_agent (37.137 ms) : 36831, 37444
. : milestone, 37137,
appsec (46.805 ms) : 46397, 47214
. : milestone, 46805,
code_origins (46.979 ms) : 46547, 47412
. : milestone, 46979,
iast (43.028 ms) : 42658, 43399
. : milestone, 43028,
profiling (47.869 ms) : 47442, 48296
. : milestone, 47869,
tracing (44.498 ms) : 44113, 44884
. : milestone, 44498,
section candidate
no_agent (37.519 ms) : 37214, 37825
. : milestone, 37519,
appsec (47.553 ms) : 47119, 47986
. : milestone, 47553,
code_origins (44.52 ms) : 44138, 44901
. : milestone, 44520,
iast (45.353 ms) : 44960, 45746
. : milestone, 45353,
profiling (45.49 ms) : 45069, 45910
. : milestone, 45490,
tracing (42.464 ms) : 42114, 42814
. : milestone, 42464,
Request duration reports for insecure-bankgantt
title insecure-bank - request duration [CI 0.99] : candidate=1.51.1-SNAPSHOT~603b5851ac, baseline=1.51.1-SNAPSHOT~3a35f77b25
dateFormat X
axisFormat %s
section baseline
no_agent (4.361 ms) : 4312, 4410
. : milestone, 4361,
iast (9.592 ms) : 9428, 9756
. : milestone, 9592,
iast_FULL (13.585 ms) : 13318, 13852
. : milestone, 13585,
iast_GLOBAL (10.039 ms) : 9866, 10213
. : milestone, 10039,
profiling (8.63 ms) : 8489, 8772
. : milestone, 8630,
tracing (7.519 ms) : 7414, 7623
. : milestone, 7519,
section candidate
no_agent (4.378 ms) : 4321, 4435
. : milestone, 4378,
iast (9.245 ms) : 9095, 9395
. : milestone, 9245,
iast_FULL (13.3 ms) : 13036, 13563
. : milestone, 13300,
iast_GLOBAL (10.226 ms) : 10049, 10402
. : milestone, 10226,
profiling (8.775 ms) : 8640, 8910
. : milestone, 8775,
tracing (7.536 ms) : 7423, 7650
. : milestone, 7536,
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.51.1-SNAPSHOT~603b5851ac, baseline=1.51.1-SNAPSHOT~3a35f77b25
dateFormat X
axisFormat %s
section baseline
no_agent (15.225 s) : 15225000, 15225000
. : milestone, 15225000,
appsec (14.995 s) : 14995000, 14995000
. : milestone, 14995000,
iast (18.528 s) : 18528000, 18528000
. : milestone, 18528000,
iast_GLOBAL (18.029 s) : 18029000, 18029000
. : milestone, 18029000,
profiling (15.447 s) : 15447000, 15447000
. : milestone, 15447000,
tracing (15.111 s) : 15111000, 15111000
. : milestone, 15111000,
section candidate
no_agent (15.468 s) : 15468000, 15468000
. : milestone, 15468000,
appsec (14.922 s) : 14922000, 14922000
. : milestone, 14922000,
iast (18.651 s) : 18651000, 18651000
. : milestone, 18651000,
iast_GLOBAL (17.874 s) : 17874000, 17874000
. : milestone, 17874000,
profiling (16.009 s) : 16009000, 16009000
. : milestone, 16009000,
tracing (15.091 s) : 15091000, 15091000
. : milestone, 15091000,
Execution time for tomcatgantt
title tomcat - execution time [CI 0.99] : candidate=1.51.1-SNAPSHOT~603b5851ac, baseline=1.51.1-SNAPSHOT~3a35f77b25
dateFormat X
axisFormat %s
section baseline
no_agent (1.469 ms) : 1457, 1480
. : milestone, 1469,
appsec (3.582 ms) : 3371, 3794
. : milestone, 3582,
iast (2.19 ms) : 2127, 2252
. : milestone, 2190,
iast_GLOBAL (2.239 ms) : 2175, 2302
. : milestone, 2239,
profiling (2.021 ms) : 1971, 2071
. : milestone, 2021,
tracing (2.018 ms) : 1969, 2067
. : milestone, 2018,
section candidate
no_agent (1.472 ms) : 1460, 1483
. : milestone, 1472,
appsec (3.583 ms) : 3371, 3795
. : milestone, 3583,
iast (2.197 ms) : 2135, 2260
. : milestone, 2197,
iast_GLOBAL (2.234 ms) : 2171, 2297
. : milestone, 2234,
profiling (2.032 ms) : 1982, 2082
. : milestone, 2032,
tracing (2.005 ms) : 1956, 2054
. : milestone, 2005,
|
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.
public static final class ConstructorAdvice { | ||
@Advice.OnMethodExit | ||
public static <T> void captureScope( | ||
@Advice.This Object task // StructuredTaskScopeImpl.SubtaskImpl (can't use the type) |
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.
Can you put a bit more description in comment abut why we can't?
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.
This is general rule of thumb for instrumentation development (they usually don't even put comment)
There are few reasons but in this case, the advice are compile against Java 8 so the type from JDK25 can't be referred.
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.
So, let's just put it as
// StructuredTaskScopeImpl.SubtaskImpl (the advice are compile against Java 8 so the type from JDK25 can't be referred).
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.
Sure, if it brings clarity, I will add it 👍
@@ -4,4 +4,4 @@ org.gradle.jvmargs=-XX:MaxMetaspaceSize=1g | |||
org.gradle.java.installations.auto-detect=false | |||
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.
What Does This Do
This PR brings support for the Structured Concurrency API on JDK 25
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
Relates to the JEP 505
Contributor Checklist
type:
and (comp:
orinst:
) labels in addition to any usefull labelsclose
,fix
or any linking keywords when referencing an issue.Use
solves
instead, and assign the PR milestone to the issueJira ticket: LANGPLAT-702