Repository navigation
PROF-16527: fix inverted jniExceptionCheck() guards in HeapUsage::initJMXUsage() - #947
zhengyu123 wants to merge 5 commits into
Conversation
…tJMXUsage() jniExceptionCheck() returns true when an exception was pending, but every guard in initJMXUsage() returned on false, i.e. whenever the JNI call succeeded. The JMX heap-usage fallback never initialized and nothing called MemoryImpl.getMemoryUsage0, so VMStructs::NativeMethodBind never captured it. - Invert the guards. - Call initJMXUsage() from VMInit only when the native binding is intercepted (HeapUsage::needsNativeBindingInterception()); with the guards fixed, every other JVM would otherwise start loading JMX at startup. - Bound the delayed NativeMethodBind list (512 pairs, no bounds check). - heapUsage_ut drives initJMXUsage() through a fake JNIEnv. - JvmtiEventTimingTest.agentOnLoadSeesNativeBindingsBeforeFirstStart checks getMemoryUsage0 is bound where interception is needed (musl JDK 11). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Scan-Build Report
Bug Summary
Reports
|
|||||||||||||||||||||||||||||||||||||||||||||||||
An agent that failed before printing the NativeMethodBind interception marker left inTime true and the interception flag false, so the test skipped. Require exit code 0 and one of the two markers, and skip only on the explicit "disabled" marker. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
CI Test ResultsRun: #42295008440 | Commit:
Status Overview
Legend: ✅ passed | ❌ failed | ⚪ skipped | 🚫 cancelled Summary: Total: 32 | Passed: 32 | Failed: 0 Updated: 2026-11-06 21:37:27 UTC |
… refs With the jniExceptionCheck() guards fixed, VMInit's initJMXUsage() call really ran ManagementFactory/getMemoryMXBean/getHeapMemoryUsage before main(). Nothing reads the JMX heap usage: HeapUsage::get(true) only takes the JMX branch when VMStructs can't read the heap, and its sole caller uses it only when VMStructs can; GC callbacks pass allow_jmx=false. Drop the VMInit call so startup never loads JMX. - initJMXUsage() deletes its JNI local refs on every exit path. - heapUsage_ut checks no local ref leaks on success, throw or null. - JvmtiEventTimingTest now asserts getMemoryUsage0 is not bound at startup (agentStartupDoesNotBootstrapJmx). - Correct the delayed-binding comment; mark the test-only friend. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…NIEnv - LocalRefs deletes copy operations and asserts on capacity overflow. - initJMXUsage(nullptr) returns without consuming the once-only attempt. - heapUsage_ut covers the null-JNIEnv case. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…flags Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Reliability & Chaos Results✅ All reliability & chaos checks passed Pipeline: https://gitlab.ddbuild.io/DataDog/java-profiler/-/pipelines/151141206 |
Codex Review Summary: ✅ CompletedThis 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. |
![datadog-datadog-prod-us1[bot]](https://avatars.githubusercontent.com/u/365230?s=60&v=4){kind=small}
There was a problem hiding this comment.
More details
Static inspection supports removing startup JMX initialization while preserving the heap-usage path used by current callers. Runtime behavior on JVMs requiring native-binding interception was not exercised.
🤖 Bits Code Review · Commit 9a354fe
![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: 9a354fe6af
ℹ️ 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".
| _lock.lock(); | ||
| if (delayed != NULL) { | ||
| // Past the capacity a binding is dropped rather than overflowing. | ||
| if (delayed != NULL && delayedCounter < MAX_DELAYED * 2) { |
There was a problem hiding this comment.
When more than 512 bindings arrive before a JNIEnv is available, this guard prevents writes past the allocated array, but none of the added tests exercises NativeMethodBind or verifies that the 513th pair is dropped. Add a boundary test for the first rejected pair so a regression in this overflow protection is caught.
AGENTS.md reference: AGENTS.md:L381-L385
Useful? React with 👍 / 👎.
jniExceptionCheck() returns true when an exception was pending, but every guard in initJMXUsage() returned on false, i.e. whenever the JNI call succeeded. initJMXUsage() therefore returned right after its first FindClass and never reached the JMX calls.
Fixing the guards would make VMInit's initJMXUsage() call actually bootstrap JMX (ManagementFactory, getMemoryMXBean, getHeapMemoryUsage) before main(). Nothing reads that result: HeapUsage::get(true) takes the JMX branch only when VMStructs cannot read the heap, its sole caller uses it only when VMStructs can, and GC callbacks pass allow_jmx=false. So VMInit no longer calls it at all.
What does this PR do?:
Fixes the inverted exception checks in HeapUsage::initJMXUsage() and stops the agent from bootstrapping JMX at VMInit.
Motivation:
PROF-16527. With the guards fixed, VMInit would have loaded JMX on every JVM that needs native-binding interception (JDK 8 HotSpot, musl JDK 11, Zing) for a value nothing uses. Early JMX initialization can also interfere with containers such as JBoss/WildFly that configure JMX themselves.
Additional Notes:
/code-reviewof the fix commits found nothing further that was reachable.How to test the change?:
./.claude/commands/build-and-summarize gtestDebug_heapUsage_ut gtestRelease_heapUsage_ut: 4 tests, passing../.claude/commands/build-and-summarize :ddprof-test:testDebug -Ptests=JvmtiEventTimingTest: asserts only on JVMs that intercept native bindings (e.g. musl JDK 11). It skips on JDKs that don't, including Zulu 8 on macOS.For Datadog employees:
credentials of any kind, I've requested a security review (run the
dd:platform-security-reviewskill, or file a request via the PSEC review form).
bewairealso runs automatically on every PR.Unsure? Have a question? Request a review!
🤖 Generated with Claude Code