Fix instrumenting metric probes for Kotlin#11890
Conversation
track objects on the stack with frame for each instruction using ASM analyzer. based on this generate POP/POP2 bytecode instruction to leave the stack clean before inserting metric calls. Kotlin compiler can, especially for suspend functions, generates a state machine with duplicate values on the stack while a return instruction will clear them (dropping the frame). we are previously relying on the fact that the number of values on the stack are precise even before a return instruction which leads to introduce jump to a same location but without the same number of values on the stack, triggering the verifier.
|
Hi! 👋 Thanks for your pull request! 🎉 To help us review it, please make sure to:
If you need help, please check our contributing guidelines. |
![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: b063fcae74
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| when(config.isDynamicInstrumentationEnabled()).thenReturn(true); | ||
| when(config.isDynamicInstrumentationClassFileDumpEnabled()).thenReturn(true); | ||
| when(config.isDynamicInstrumentationVerifyByteCode()).thenReturn(true); | ||
| // when(config.isDynamicInstrumentationVerifyByteCode()).thenReturn(true); |
There was a problem hiding this comment.
Because Mockito returns false for unstubbed booleans, commenting out this stub disables DebuggerTransformer.verifyByteCode for every metric-probe instrumentation test, while the product default for DD_DYNAMIC_INSTRUMENTATION_VERIFY_BYTECODE is enabled. The Kotlin exit-probe change is specifically verifier-sensitive, so these tests can now pass with bytecode that would be rejected in the default production path; keep verification enabled or scope any opt-out to a documented exceptional case.
Useful? React with 👍 / 👎.
This comment has been minimized.
This comment has been minimized.
Debugger benchmarksParameters
See matching parameters
SummaryFound 0 performance improvements and 0 performance regressions! Performance is the same for 10 metrics, 5 unstable metrics. See unchanged results
Request duration reports for reportsgantt
title reports - request duration [CI 0.99] : candidate=None, baseline=None
dateFormat X
axisFormat %s
section baseline
noprobe (349.243 µs) : 293, 405
. : milestone, 349,
basic (296.531 µs) : 289, 304
. : milestone, 297,
loop (8.98 ms) : 8974, 8986
. : milestone, 8980,
section candidate
noprobe (332.241 µs) : 307, 358
. : milestone, 332,
basic (296.453 µs) : 290, 303
. : milestone, 296,
loop (8.983 ms) : 8977, 8989
. : milestone, 8983,
|
🟡 Java Benchmark SLOs — Performance SLO warning (near threshold)
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. |
| } | ||
| } catch (AnalyzerException ex) { | ||
| LOGGER.debug("Failed to analyze method[%s::%s] instructions", owner, methodNode.name, ex); | ||
| } |
There was a problem hiding this comment.
Can this produce a half-finished map? Would that be an issue?
There was a problem hiding this comment.
No because the specific exception is thrown only by ASM's Analyzer.analyze method so when the exception is thrown, the map is untouchly empty
|
|
||
| @Override | ||
| protected InsnList getBeforeReturnInsnList(AbstractInsnNode node) { | ||
| protected InsnList getBeforeReturnInsnList( |
There was a problem hiding this comment.
Are we sure this is limited to metric probes?
There was a problem hiding this comment.
Until contrary proven, yes because this is specific to the code inserted for metric probe instrumentation.
Though, it is not excluded to manifest in other instrumentation cases.
This is why we will add exploration tests for Kotlin code
|
/merge |
|
View all feedbacks in Devflow UI.
The expected merge time in
|
gh-worker-dd-mergequeue-cf854d
Bot
merged commit a4087b6
into
master
gh-worker-dd-mergequeue-cf854d
Bot
deleted the
jpbempel/fix-metric-suspend-fun
branch
2 participants
What Does This Do
track objects on the stack with frame for each instruction using ASM analyzer. based on this generate POP/POP2 bytecode instruction to leave the stack clean before inserting metric calls.
Motivation
Kotlin compiler can, especially for suspend functions, generates a state machine with duplicate values on the stack while a return instruction will clear them (dropping the frame).
we are previously relying on the fact that the number of values on the stack are precise even before a return instruction which leads to introduce jump to a same location but without the same number of values on the stack, triggering the verifier.
Additional Notes
Claude analysis:
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 issue/merge. You can also:/merge --commit-message "..."/merge -c/merge -f --reason "reason"; please use this judiciously, as some checks do not run at the PR-level (note: the PR still needs to be mergeable, this will only skip the pre-merge build)Jira ticket: DEBUG-5816