Instrument LambdaMetafactory and preserve Runnable lambda identity during context propagation - #12346
Instrument LambdaMetafactory and preserve Runnable lambda identity during context propagation#12346amarziali wants to merge 23 commits into
Conversation
|
@DataDog review |
This comment has been minimized.
This comment has been minimized.
![datadog-prod-us1-6[bot]](https://avatars.githubusercontent.com/u/365230?s=60&v=4){kind=small}
There was a problem hiding this comment.
On JDK 8 and JDK 11, the new structure matcher requires a field that does not exist. The lambda transformer does not run, and Runnable lambdas still use wrappers.
🤖 Datadog Autotest · Commit 054841c · What is Autotest? · @DataDog review to ask questions · Any feedback? Reach out in #autotest
054841c to
5742c20
Compare
|
@DataDog review |
There was a problem hiding this comment.
More details
The PR instruments generated Runnable lambdas so executor propagation keeps task identity, with supported JDK paths and a Graal build guard. This update fixes the earlier JDK 8 and 11 samBase mismatch and adds no new finding; the documented concurrent-reuse limitation remains, and no review finding is open.
🤖 Datadog Autotest · Commit 5655a6f · What is Autotest? · @DataDog review to ask questions · Any feedback? Reach out in #autotest
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Codex Review: Didn't find any major issues. Breezy! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
🟢 Java Benchmark SLOs — All performance SLOs passed
PR vs. master results
Commit: Load and DaCapo benchmarks can be triggered manually in the GitLab pipeline. Results will appear in the Benchmarking Platform UI after completion. |
|
Hi! 👋 Thanks for your pull request! 🎉 To help us review it, please make sure to:
If you need help, please check our contributing guidelines. |
amarziali
added
inst: java
|
@DataDog review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a721063dff
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if ((opcode == Opcodes.INVOKEVIRTUAL | ||
| && "toByteArray".equals(name) | ||
| && "()[B".equals(descriptor) | ||
| && owner.endsWith("/ClassWriter")) |
There was a problem hiding this comment.
When CDS/AppCDS already contains a lambda proxy, spinInnerClass() can return the class obtained from LambdaProxyClassArchive.find(...) before either byte-generation call matched here executes. Consequently the helper never transforms that Runnable; executor propagation falls back to wrapping it, so task identity is still changed in CDS-enabled applications—the behavior this change is intended to eliminate.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
That's true. Right now this is fine meaning that for CDS there will be a fallback to wrapping. I'm more incline to evaluate that in a separate PR.
| if (!(task instanceof RunnableWrapper) | ||
| && !(task instanceof FieldBackedContextAccessor) | ||
| && !ExcludeFilter.exclude(ExcludeType.RUNNABLE, task)) { |
There was a problem hiding this comment.
When the same lambda instance is submitted more than once before its first execution consumes the stored continuation—for example, a static or non-capturing Runnable queued concurrently by two requests—skipping the wrapper makes every submission share one State slot. The second capture is rejected, and overlapping executions can either lose a parent or both resume the first submission's continuation, producing missing or incorrectly parented traces; this path needs per-submission state rather than opting every field-backed lambda out of wrapping.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
As I explcitly already put in the PR description for reviewers:
Previously, wrapped lambdas had per-submission state. This limitation is most relevant to reusable singleton or stateless lambdas submitted concurrently; lambdas created per operation are unlikely to encounter it.
This is a known limitation and already existing for all the other field injected classes that are going throught threadpool. It's not introduced by this change and can be considered a trade-off we can perhaps live together for now.
There was a problem hiding this comment.
A hidden Runnable lambda can put its type description in the shared name cache. A later real class with the same legal binary name can receive wrong instrumentation on JDK 21 and later.
🤖 Datadog Autotest · Commit a721063 · What is Autotest? · @DataDog review to ask questions · Any feedback? Reach out in #autotest
| SharedTypeInfo<TypeDescription> sharedType = types.find(name); | ||
| // Same-owner lambdas share a symbolic name, so build their target from the supplied bytes. | ||
| SharedTypeInfo<TypeDescription> sharedType = | ||
| transformingLambda && name.equals(targetName) ? null : types.find(name); |
There was a problem hiding this comment.
The agent can apply wrong instrumentation to the normal class.
Assertion details
- Input: On JDK 21 or later, transform a hidden Runnable lambda for Foo, then define Foo.$Lambda in the same class loader.
- Expected:
The transformer must parse the normal class from its own bytecode. - Actual:
The normal transformation reuses the hidden lambda type description from the shared name cache.
Was this helpful? React 👍 or 👎
🤖 Datadog Autotest · What is Autotest? · @DataDog review to ask questions · Any feedback? Reach out in #autotest
There was a problem hiding this comment.
it's a good catch. I initially wanted to try not to touch this part as well but it seems something that can happen. A dedicate lambda matcher and a cache correctness fix have been done in 1bc6bbc . the perf test looks untouched
|
@DataDog review |
There was a problem hiding this comment.
The new module has no Gradle lock file. Its dependency graph can change between builds.
🤖 Datadog Autotest · Commit b11c8c6 · What is Autotest? · @DataDog review to ask questions · Any feedback? Reach out in #autotest
|
@DataDog review |
There was a problem hiding this comment.
Disabled Runnable instrumentation still gets the new lambda transform cost. AppCDS and restricted SecurityManager use also skip the identity-preserving path.
🤖 Datadog Autotest · Commit 35be726 · What is Autotest? · @DataDog review to ask questions · Any feedback? Reach out in #autotest
| super.visitMethodInsn(opcode, owner, name, descriptor, isInterface); | ||
| // Match repackaged JDK APIs while excluding unrelated byte-array producers. The generated | ||
| // byte[] remains on the operand stack after the original call. | ||
| if ((opcode == Opcodes.INVOKEVIRTUAL |
There was a problem hiding this comment.
AppCDS applications keep the old Runnable wrapper and do not preserve task identity.
Assertion details
- Input: Load a Runnable lambda from an AppCDS archive that was made without the agent.
- Expected:
The agent must preserve Runnable identity for saved lambda classes or state this limit. - Actual:
The visitor only changes newly built class bytes. An AppCDS archive hit returns the saved lambda class before these calls.
Was this helpful? React 👍 or 👎
🤖 Datadog Autotest · What is Autotest? · @DataDog review to ask questions · Any feedback? Reach out in #autotest · Open Bits AI session
There was a problem hiding this comment.
Archived lambda classes can bypass the normal generation path. Forcing transformation would reduce CDS benefits. Wrapping preserves propagation, although identity changes. It's a known limitation already documented and I don't think we should fix now
| TypePoolFacade.beginLambdaTransform(interfaceClassName); | ||
| try { | ||
| return classFileTransformer.transform( | ||
| targetClass.getClassLoader(), |
There was a problem hiding this comment.
Affected applications keep the old wrapper and do not preserve Runnable identity.
Assertion details
- Input: Link an application Runnable lambda when an active SecurityManager denies getClassLoader or getProtectionDomain access.
- Expected:
The agent must read the class data with agent access rights. - Actual:
The class access calls throw SecurityException. The catch returns the original bytes and stops lambda instrumentation.
Was this helpful? React 👍 or 👎
🤖 Datadog Autotest · What is Autotest? · @DataDog review to ask questions · Any feedback? Reach out in #autotest · Open Bits AI session
There was a problem hiding this comment.
Accessing the target class’s protection domain can be denied. The code safely catches this and falls back. SecurityManager usage is increasingly rare, and IMHO changing the protection domain handling would be a risky change. I would steer not to fix it
|
@DataDog review |
There was a problem hiding this comment.
This PR instruments Runnable lambdas before class definition to keep task identity.
The new commit removes full transform work when no lambda matcher is active.
AppCDS archive hits still bypass this instrumentation.
SecurityManager restrictions can still block class metadata access and cause wrapper fallback.
This increment adds no new reportable issue.
Was this helpful? React 👍 or 👎
🤖 Datadog Autotest · Commit 0c05100 · What is Autotest? · @DataDog review to ask questions · Any feedback? Reach out in #autotest
|
Question on My understanding is that Native Image's AOT compilation doesn't actually route an app's lambda call sites through the real The case where it seems like it would actually matter is narrower: the Is that the actual scenario driving this guard, or is there something else about how Native Image touches |
dougqh
left a comment
dougqh
left a comment
There was a problem hiding this comment.
A couple of things I'd flag while reviewing the LambdaMetafactory instrumentation (see thread for context on how these were derived).
| public void visitEnd() { | ||
| super.visitEnd(); | ||
| if (!injected) { | ||
| log.debug( |
There was a problem hiding this comment.
This whole feature depends on matching a specific bytecode shape inside spinInnerClass/generateInnerClass that is tied to JDK internals (the comment above already notes it changed for JDK 25). When the shape doesn't match on some future JDK, the only signal is a debug-level log line here — field-injection for Runnable lambdas silently falls back to the wrap-based path with nothing visible in normal operation. Given this is exactly the kind of JDK-version regression that's easy to miss until a customer notices missing spans, would it be worth a one-time higher-severity log (or a startup self-test against the running JDK) instead of log.debug?
|
|
||
| @Override | ||
| public ElementMatcher<TypeDescription> lambdaMatcher() { | ||
| return notExcludedByName(RUNNABLE); |
There was a problem hiding this comment.
lambdaMatcher() and hierarchyMatcher() both start from notExcludedByName(RUNNABLE) independently. If one is tightened or loosened later (e.g. to exclude another framework's Runnable implementations) without updating the other, the metafactory-injection path and the normal hierarchy-instrumentation path would silently diverge on which Runnables they cover. Might be worth deriving one from the other (or a shared base matcher) so they can't drift apart.
dougqh
left a comment
dougqh
left a comment
There was a problem hiding this comment.
Follow-up review with the findings from the earlier automated pass (recovered from local scrollback after the first set I posted was reconstructed from a fuzzier summary). All 10 are re-verified here against the current head commit. The dominant pattern: since hidden lambda classes can never be retransformed later (wouldIgnore explicitly excludes names containing /), several distinct races/re-entrancy edges cause a silent, permanent loss of field-injection for individual lambdas — they quietly fall back to RunnableWrapper, which is safe but defeats this PR's own goal of identity preservation for the affected lambdas, with only a debug log to notice it by.
| ids.clear(); | ||
|
|
||
| long fromTick = InstrumenterMetrics.tick(); | ||
| String lambdaInterface = TypePoolFacade.lambdaInterface(); |
There was a problem hiding this comment.
A lambda linked while EXPERIMENTAL_DEFER_INTEGRATIONS_UNTIL deferral is active returns false here and permanently skips field-injection: wouldIgnore() (line 193) explicitly excludes names containing / ("don't retransform lambdas"), so the later resumeMatching() retransform sweep never revisits it. That's a permanent, silent miss for any lambda linked during the deferral window, not just a delayed one.
| return classBytes; | ||
| } | ||
| if (Boolean.TRUE.equals(TRANSFORMING.get())) { | ||
| log.debug("Lambda {} skipped: re-entrant transform", lambdaClassName); |
There was a problem hiding this comment.
The TRANSFORMING re-entrancy guard drops field-injection for any lambda whose class definition is triggered as a side effect of processing another lambda's transform on the same thread (e.g. a lambda captured while defining/loading another lambda's supporting classes). That lambda permanently falls back to RunnableWrapper with only this debug log as a trace.
| try { | ||
| return transformerBuilder.installOn(inst); | ||
| ClassFileTransformer classFileTransformer = transformerBuilder.installOn(inst); | ||
| registerLambdaTransformer(classFileTransformer, transformerBuilder.lambdaInterfaces()); |
There was a problem hiding this comment.
Narrow startup race: transformerBuilder.installOn(inst) (which can trigger retransformation) runs before registerLambdaTransformer calls LambdaTransformerHolder.set(...). A lambda linked in that window gets a null transformer in LambdaTransformerHelper.transform and permanently misses injection — there's no retry once the window closes.
| return deferredTypes.computeIfAbsent(name, deferType); | ||
| } | ||
|
|
||
| private boolean isLambdaTarget(String name) { |
There was a problem hiding this comment.
isLambdaTarget compares against targetName, a per-thread field that's set for the duration of beginLambdaTransform/endLambdaTransform but isn't scoped to a single lookup call. If resolving the lambda's type triggers resolution of an unrelated class on the same thread while the lambda transform is still "open" (e.g. an advice helper class loaded as a side effect), that unrelated class could get routed onto the lambda-only matching path in CombiningMatcher and skip its own normal instrumentation.
| LambdaMatchRecorder[] recorders = lambdaMatchers.get(lambdaInterface); | ||
| if (null != recorders) { | ||
| for (LambdaMatchRecorder recorder : recorders) { | ||
| recorder.record(target, classLoader, ids); |
There was a problem hiding this comment.
recorder.record(target, classLoader, ids) here isn't wrapped in try/catch, unlike the equivalent loop over regular MatchRecorders a few lines below (line 111-112, which logs and continues on Throwable). An exception from a LambdaMatchRecorder propagates uncaught instead of being handled the same way.
| // Anonymous classes have '/' in class name which is not allowed in 'normal' classes. | ||
| // Field-injected tasks are already instrumented and must retain their identity. | ||
| if (!(task instanceof RunnableWrapper) | ||
| && !(task instanceof FieldBackedContextAccessor) |
There was a problem hiding this comment.
instanceof FieldBackedContextAccessor is used here as a proxy for "already has propagation fields," but that marker isn't scoped to a specific context store. If a future second ForLambda instrumenter targets Runnable with a different context store, this check would treat its lambdas as already handled and skip wrapping them too, silently dropping propagation for that store with no fallback.
| # matching and transformation pipeline. Keep this list narrow: the lookup runs for every lambda | ||
| # linkage in the application. | ||
|
|
||
| 1 java.lang.Runnable |
There was a problem hiding this comment.
This allowlist is duplicated between this trie and each ForLambda instrumenter's runtime lambdaInterface() (and the array built from them). Nothing enforces they stay in sync — forgetting to add a new interface here while adding a new ForLambda instrumenter would make LambdaInterfaceNameTrie.apply return something other than 1, silently no-oping the new instrumentation instead of failing loudly.
| boolean isOutline = typeParser == outlineTypeParser; | ||
| long fromTick = InstrumenterMetrics.tick(); | ||
| // Hidden lambda names may later be reused by an ordinary class definition. | ||
| boolean cacheable = !isLambdaTarget(name); |
There was a problem hiding this comment.
"Don't cache under a lambda's transient hidden-class name" is reimplemented ad hoc at this call site and several others (line 394 and the corresponding guards in Memoizer/WithLocation) rather than enforced once at the cache boundary. It's currently consistent, but every new cache added to this area has to remember to add its own isLambdaTarget/isCacheable check rather than getting it for free.
| return null; | ||
| } | ||
| return (className, targetClass, classBytes, interfaceName) -> { | ||
| for (String enabledInterface : lambdaInterfaces) { |
There was a problem hiding this comment.
filterLambdaTransformer does a linear scan over lambdaInterfaces per lambda link, on exactly the hot path the PR's own JMH benchmark (LambdaExecutorBenchmark) measures. CombiningTransformerBuilder already has these as a Set/keyed map before it gets flattened to this array — reusing that instead of a per-call linear scan would avoid the O(n) lookup for every lambda linkage.
| @SuppressWarnings("unchecked") | ||
| private static LambdaTransformer newLambdaTransformer( | ||
| final ClassFileTransformer classFileTransformer) { | ||
| if (JavaVirtualMachine.isJavaVersionAtLeast(9)) { |
There was a problem hiding this comment.
This JDK9+ reflective-loading logic duplicates the existing idiom used elsewhere in this class/AgentStrategies for loading version-specific helper classes reflectively. Worth sharing that helper instead of a second bespoke reflection path.
2 participants
What Does This Do
Instrument
Runnablelambda classes when they are generated byLambdaMetafactory, allowing executor context propagation to attach state directly to the lambda instead of creating aRunnableWrapper.This is primarily a functional improvement: avoiding the wrapper preserves the submitted task’s identity while retaining the existing context-propagation behavior.
Notable changes
java.lang.Runnable; the mechanism can later be extended to interfaces such asCallable.dd.trace.lambda.enabled=falseavailable as an opt-out.Known limitation
Concurrent submissions of the same
Runnablelambda instance share a single continuation slot. As with other field-injected Runnable implementations, only one pending context can be retained, so overlapping submissions may lose or misassociate context propagation.Previously, wrapped lambdas had per-submission state. This limitation is most relevant to reusable singleton or stateless lambdas submitted concurrently; lambdas created per operation are unlikely to encounter it.
Benchmarks
Benchmarks were run with
dd.benchmark.enabled=true.Spring Boot startup
A Spring Boot 3.3 Petclinic-style application was measured on Temurin 17 across 15 interleaved runs:
The 95% confidence interval for the paired difference was ±43.9 ms, so the measured startup difference was indistinguishable from run-to-run noise.
The application generated approximately 1,600 lambda classes. Of those, 22 exact-
Runnablecandidates entered the transformer and 8 were modified.JMH results
run(), JDK 17run(), JDK 25The linkage cost is paid once when a matching lambda call site is created. Unrelated lambda interfaces take the fast trie-rejection path. The direct-run benchmark intentionally uses a nearly empty Runnable, making the relative percentage appear larger than the very small absolute overhead.
GC profiling confirms an 8-byte increase for instrumented capturing Runnable lambda instances due to the injected state field. This is the expected heap tradeoff and may be more visible in applications that retain large numbers of Runnable objects.
Motivation
Additional Notes
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]