Don't preload jmethodIDs when cstack=vm with hotspot JVM#549
Don't preload jmethodIDs when cstack=vm with hotspot JVM#549zhengyu123 wants to merge 23 commits into
Conversation
|
CI Test ResultsRun: #26957560544 | Commit:
Status Overview
Legend: ✅ passed | ❌ failed | ⚪ skipped | 🚫 cancelled Failed Testsmusl-amd64/debug / 8-librcaJob: View logs No detailed failure information available. Check the job logs. musl-amd64/debug / 11-librcaJob: View logs No detailed failure information available. Check the job logs. musl-amd64/debug / 21-librcaJob: View logs No detailed failure information available. Check the job logs. musl-amd64/debug / 25-librcaJob: View logs No detailed failure information available. Check the job logs. musl-amd64/debug / 17-librcaJob: View logs No detailed failure information available. Check the job logs. Summary: Total: 32 | Passed: 27 | Failed: 5 Updated: 2026-06-04 14:33:26 UTC |
Scan-Build Report
Bug Summary
Reports
|
||||||||||||||||||||||||||||||||||||
![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: 13349c38df
ℹ️ 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".
| return method->validatedId(); | ||
| } | ||
| return NULL; | ||
| return JMETHODID_NOT_WALKABLE; |
There was a problem hiding this comment.
When getMethodId rejects an unreadable or misaligned VMMethod*, it now returns the non-null JMETHODID_NOT_WALKABLE sentinel. Several existing recovery paths still guard only with if (method_id != NULL) before dereferencing the original method pointer (for example the anchor recovery paths later in walkVM), so corrupted interpreter-frame data now passes the guard and reaches method->bytecode() instead of being skipped as before. This can turn the defensive validation into a crash on the exact invalid-frame cases it is meant to protect.
Useful? React with 👍 / 👎.
zhengyu123
changed the title
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b95c88c66f
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: df5e992559
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c26aeea66e
ℹ️ 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".
| } | ||
|
|
||
| bool HotspotSupport::loadMethodIDsImpl(jvmtiEnv *jvmti, JNIEnv *jni, jclass klass) { | ||
| jobject cl; |
There was a problem hiding this comment.
When force_jmethodID=false sees a hidden class, !isHiddenClass(...) is false, so GetClassLoader is never called and cl remains uninitialized before the cleanup check below. Hidden classes are common on modern HotSpot (e.g. lambdas/MethodHandles), so this can randomly call DeleteLocalRef on garbage or skip cleanup during class prepare / startup preloading instead of safely falling through to GetClassMethods.
Useful? React with 👍 / 👎.
jbachorik
left a comment
jbachorik
left a comment
There was a problem hiding this comment.
Please add tests for those changes. They are complex enough to warrant specialized tests. Thanks!
| "getPlatformClassLoader", | ||
| "()Ljava/lang/ClassLoader;"); | ||
| // JDK8 does not have platform class loader | ||
| if (getPlatformLoaderMethod != nullptr) { |
There was a problem hiding this comment.
Shouldn't this be getPlatfomrLoaderMethod == nullptr?
There was a problem hiding this comment.
Good catch! Fixed
| if (method_id != nullptr) { | ||
| fillFrame(frames[depth++], FRAME_INTERPRETED, bci, method_id); | ||
| } else { | ||
| fillFrameRaw(frames[depth++], FRAME_INTERPRETED, bci, method); | ||
| } |
There was a problem hiding this comment.
I see this pattern repeating quite a lot - would it make sense to extract a function and have the logic in one place?
|
|
||
| bool HotspotSupport::loadMethodIDsImpl(jvmtiEnv *jvmti, JNIEnv *jni, jclass klass) { | ||
| jobject cl; | ||
| // Hotspot only: none hidden classes loaded by system class loaders, which are never unloaded, |
There was a problem hiding this comment.
Nit
| // Hotspot only: none hidden classes loaded by system class loaders, which are never unloaded, | |
| // Hotspot only: no hidden classes loaded by system class loaders, which are never unloaded, |
| if (method_id != nullptr && method_id != JMETHODID_NOT_WALKABLE) { | ||
| return method_id; | ||
| } |
There was a problem hiding this comment.
Also, maybe useful to have a function or macro for method_id != nullptr && method_id != JMETHODID_NOT_WALKABLE condition, which seems to be used quite a lot
| static name * load_then_cast(const void* ptr) { \ | ||
| assert(ptr != nullptr); \ | ||
| return cast(*(const void**)ptr); } | ||
| if (ptr == nullptr) return nullptr; \ |
There was a problem hiding this comment.
Wouldn't cast_or_null(ptr) just do the right thing when ptr == nullptr?
|
|
||
| #include <jvmti.h> | ||
|
|
||
| #include "frame.h" |
There was a problem hiding this comment.
Was this intentional? Required by toolchain?
There was a problem hiding this comment.
Yes. It used to inherit it from`vmEntry.h", but now not included.
| msg = "jstackdepth must be > 0"; | ||
| } | ||
|
|
||
| CASE("force_jmethodID") |
There was a problem hiding this comment.
| CASE("force_jmethodID") | |
| CASE("fjmethodid") |
Something a bit easier to type. And add it to the args description with some details at the top of this file.
jbachorik
left a comment
jbachorik
left a comment
There was a problem hiding this comment.
Sphinx review findings (lines not in diff)
[MEDIUM] hotspof-lib/src/main/cpp/hotspot/hotspotSupport.cpp:680 · completeness
Direct VMMethod::id() callers at lines 680, 1089–1100, and 139 (fillFrameTypes) were not updated for the new JMETHODID_NOT_WALKABLE sentinel. Their != NULL guards now let the sentinel through, emitting a spurious frame that later collapses to "unknown"; the PROBE_SP retract logic at 1098 also misfires.
if (method_id != NULL) { fillFrame(..., method_id); } // ← sentinel passes this guard
Fix: method_id != NULL && method_id != JMETHODID_NOT_WALKABLE at all direct id() call sites.
[MEDIUM] ddprof-lib/src/main/cpp/hotspot/vmStructs.h:928 · correctness (latent)
isStaleMethodId compares vm_method->id() == NULL, but id() now returns JMETHODID_NOT_WALKABLE on the bogus path. The staleness predicate (JDK-8313816 workaround) is logically broken. Currently zero callers, but worth fixing alongside the sentinel migration.
return vm_method == NULL || vm_method->id() == NULL; // ← sentinel is not NULLFix: compare against both NULL and JMETHODID_NOT_WALKABLE.
[LOW / test-adequacy] ddprof-lib/src/main/cpp/frame.h · FrameType encode/decode/isRawPointer
No test covers the new bit-manipulation logic. Five mutants survive: inverting rawPointer ?, removing the mask OR, flipping & ~RAW_POINTER_MASK to & RAW_POINTER_MASK, bci > 0 → bci >= 0, and != 0 → == 0. Suggest adding a round-trip unit test asserting the type, the raw-pointer bit, and that isRawPointer(0) == false.
| } | ||
|
|
||
| bool HotspotSupport::loadMethodIDsImpl(jvmtiEnv *jvmti, JNIEnv *jni, jclass klass) { | ||
| jobject cl; |
There was a problem hiding this comment.
[HIGH / correctness] jobject cl is uninitialized. When isHiddenClass(jvmti, klass) returns true the && short-circuits, GetClassLoader never runs, and cl retains an indeterminate stack value. Control then reaches line 1309's if (cl != nullptr) jni->DeleteLocalRef(cl); — UB, can corrupt the local-ref table or crash the JVM. Hidden classes (lambda forms, MethodHandle adapters) are routine on JDK 15+.
jobject cl = nullptr; // fix| return (int16_t)SafeAccess::load32((int32_t*)at(_constmethod_idnum_offset), -1); | ||
| } | ||
|
|
||
| u16 VMConstMethod::nameIndex() const { |
There was a problem hiding this comment.
[HIGH / robustness] nameIndex() and signatureIndex() use raw *(u16*) dereferences with no SafeAccess. These are called from HotspotSupport::resolve() at dump time — outside any setjmp/crash-protection window — on a VMMethod* captured earlier in a signal handler. If the class unloaded before finishChunk() started, this is a raw read on freed memory. The sibling idnum() on the same struct already does the right thing:
// idnum (correct)
return (int16_t)SafeAccess::load32((int32_t*)at(_constmethod_idnum_offset), -1);
// fix nameIndex / signatureIndex the same way:
return (u16)SafeAccess::load32((int32_t*)at(_constmethod_name_index_offset), 0);
return (u16)SafeAccess::load32((int32_t*)at(_constmethod_sig_index_offset), 0);A return of 0 is an invalid CP index; symbolAt(0) returns nullptr, which resolve() already handles.
There was a problem hiding this comment.
This is intentional. When casting to VMConstMethod, it ensures that the memory is readable.
| jmethodID method = frame.method_id; | ||
| // Resolve native method | ||
| if (FrameType::isRawPointer(bci)) { | ||
| method_id = JVMSupport::resolve(frame.method); |
There was a problem hiding this comment.
[MEDIUM / correctness] The raw VMMethod* stored by fillFrameRaw in the signal handler is dereferenced here at dump time. finishChunk() pins classes still loaded when it starts (via GetLoadedClasses), but classes already unloaded before that point have no JNI-reference pin — their VMMethod* is dangling. SafeAccess prevents a hard crash, but a freed-then-reallocated region that passes isReadableRange can yield a wrong (but non-crashing) method name in the recording.
Please confirm fillFrameRaw is only ever reached for classes that can never unload (bootstrap/platform/app classloader, non-lambda), and document that invariant at the call sites.
There was a problem hiding this comment.
That's what we do :-)
| if (jvmti->GetClassModifiers(clazz, &modifiers) == JVMTI_ERROR_NONE) { | ||
| // In JVM TI, hidden classes are identified by the 0x00000800 mask | ||
| // which matches the JVM_ACC_HIDDEN flag. | ||
| return (modifiers & 0x00000800) != 0; |
There was a problem hiding this comment.
[MEDIUM / consistency] 0x00000800 is documented here as JVM_ACC_HIDDEN, but in the JVMTI/class-file specification 0x0800 is ACC_STRICT (strictfp) — a method-level flag not defined for classes. The rest of this codebase detects hidden/synthetic classes via ACC_SYNTHETIC|ACC_BRIDGE = 0x1040 (see flightRecorder.h:47). A wrong hidden-class verdict routes an unloadable class to the raw-VMMethod* path.
Please verify GetClassModifiers output for actual hidden classes on JDK 15–21, or use JVMTI_CLASS_STATUS_ARRAY|JVMTI_CLASS_STATUS_PRIMITIVE / a dedicated hidden-class API, and reconcile with the existing 0x1040 constant.
There was a problem hiding this comment.
Consolidated into jvmSupport.h
| loaded_count++; | ||
| } | ||
| } | ||
| jvmti->Deallocate((unsigned char *)classes); |
There was a problem hiding this comment.
[MEDIUM / completeness] No test exercises force_jmethodID=false or verifies method-name correctness for the raw VMMethod* resolution path. A regression in resolve() — wrong class-signature format for FindClass, a failed GetMethodID, a mis-detected hidden class — would silently degrade profiles to "unknown" method names. The existing BoundMethodHandleProfilerTest only asserts event presence and counter non-emptiness.
Suggest adding an integration test that:
- Starts the profiler with
force_jmethodID=false - Profiles known system-class methods
- Asserts the recorded JFR frames contain the correct (non-
"unknown") method names
There was a problem hiding this comment.
This is a bigger test infrastructure issue, not sure how to address it right now.
| return nullptr; | ||
| } | ||
|
|
||
| if (name_sym == nullptr || sig_sym == nullptr || klass == nullptr) { |
There was a problem hiding this comment.
[LOW / necessity] klass is null-checked and returned-from at line 1343–1345, so the klass == nullptr branch in the combined condition on this line is unreachable. Remove the redundant check:
if (name_sym == nullptr || sig_sym == nullptr) { return nullptr; } // klass already guarded above| msg = "jstackdepth must be > 0"; | ||
| } | ||
|
|
||
| CASE("force_jmethodID") |
There was a problem hiding this comment.
[LOW / consistency] strncasecmp(value, "false", 5) is a prefix match — force_jmethodID=falsetto would also disable the flag. Other boolean arguments in this file use exact-match parsing. Consider using strcasecmp (exact) or at least adding a length check. No test currently covers this parsing, so a mutation of the == 0 comparison or the length 5 would go undetected.
| if (getSysLoaderMethod != nullptr) { | ||
| jobject ret = jni->CallStaticObjectMethod(classLoaderClass, getSysLoaderMethod); | ||
| if (ret != nullptr) { | ||
| JAVA_APPLICATION_CLASSLOADER = jni->NewGlobalRef(ret); |
There was a problem hiding this comment.
[LOW / robustness] The JAVA_APPLICATION_CLASSLOADER and JAVA_PLATFORM_CLASSLOADER global refs created here via NewGlobalRef are never released (DeleteGlobalRef), and the statics are never reset to nullptr on shutdown. For a singleton agent this is acceptable, but the initialize() guard at line 28 (if (JAVA_PLATFORM_CLASSLOADER != nullptr || ...)) means a re-init within the same VM (profiler restart) would silently short-circuit. Consider either documenting this as a deliberate process-lifetime choice or adding VMDeath cleanup.
| method_id = JVMSupport::resolve(frame.method); | ||
| } | ||
|
|
||
| // Maps JMETHODID_NOT_WAKEABLE back to nullptr |
There was a problem hiding this comment.
[LOW / consistency] Typo: JMETHODID_NOT_WAKEABLE → JMETHODID_NOT_WALKABLE (the constant is defined in vmEntry.h).
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b2a9a1d073
ℹ️ 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 (jvmti->GetClassModifiers(clazz, &modifiers) == JVMTI_ERROR_NONE) { | ||
| // In JVM TI, hidden classes are identified by the 0x00000800 mask | ||
| // which matches the JVM_ACC_HIDDEN flag. | ||
| return (modifiers & 0x00000800) != 0; |
There was a problem hiding this comment.
This mask does not actually identify HotSpot hidden classes: JVMTI GetClassModifiers reports JVMS/class-file access flags, where hidden lambda classes are typically final|synthetic and 0x00000800 is not a hidden-class bit. Fresh evidence since the earlier hidden-class finding is this new mask check; with fjmethodid=false, app/system-loader hidden lambdas such as $$Lambda/0x... still fall through as non-hidden/non-lambda and skip GetClassMethods, leaving unloadable methods without stable jmethodIDs for later dump-time resolution.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b56c6b47e4
ℹ️ 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".
| } | ||
|
|
||
| bool JVMSupport::loadMethodIDs(jvmtiEnv *jvmti, JNIEnv *jni, jclass klass) { | ||
| if (VM::isHotspot() && !VM::arguments()._force_jmethodID) { |
There was a problem hiding this comment.
When a HotSpot recording is started with only cstack=vm or cstack=vmx, this condition still leaves _force_jmethodID at its default true value, so VMInit/ClassPrepare continue to call the generic loadMethodIDsImpl() and preload jmethodIDs for every class. I checked the arguments.cpp cstack parser and it only sets _cstack, while the new raw-Method* path is reached only by the separate fjmethodid=false option, so the intended memory-saving behavior is not enabled for the stated cstack mode unless users know to pass an extra undocumented flag.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 756790479f
ℹ️ 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".
| _remote_symbolication(false), | ||
| _jvmtistacks(false) {} | ||
| _jvmtistacks(false), | ||
| _force_jmethodID(false) {} |
There was a problem hiding this comment.
With this default set to false, every normal HotSpot start that does not pass fjmethodid goes through HotspotSupport::loadMethodIDsImpl() and skips preloading jmethodIDs for bootstrap/platform/application classes. That raw-Method* fallback is only added in the new walkVM path; the default ASGCT-based CPU/wall stack collection still depends on preallocated jmethodIDs, so ordinary profiling modes can start returning missing/unknown Java frames for the common system-loader application methods. The field comment also says this should be true by default, so initialize it to true and only disable it for the intended cstack=vm/explicit opt-out cases.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 79adaea8da
ℹ️ 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".
| }; | ||
|
|
||
| class FrameType { | ||
| static constexpr int RAW_POINTER_MASK = 1 << 30; |
There was a problem hiding this comment.
When tiered compilation produces a FRAME_C1_COMPILED frame, FrameType::encode() already sets bit 30 because FRAME_C1_COMPILED is 6 and the type is stored as type << 25. Reusing that same bit as RAW_POINTER_MASK makes FrameType::isRawPointer() treat ordinary C1 frames as raw HotSpot Method* frames during JFR serialization, so Lookup::resolveMethod() calls JVMSupport::resolve(frame.method) on the union's jmethodID value and those C1 Java frames can become unknown or read an invalid VMMethod pointer whenever C1/tiered compilation is active.
Useful? React with 👍 / 👎.
2 participants
What does this PR do?:
Reduce memory overhead by storing raw Method pointers instead of pre-allocating jmethodIDs for classes loaded by system class loaders (bootstrap, platform and application)
This feature only applies to hotspot JVM when
cstack=vmMotivation:
Pre-allocating jmethodIDs can consume significant amount of native memory, e.g. 9G in some extreme cases.
By introducing optional lazy jmethodID resolution for system classes, HotSpot JVM profiling avoids loading jmethodIDs for every method, but only the methods on stacks, a significant optimization for native memory usage.
Additional Notes:
This feature is optional and is off by default. To enable the feature, set
force_jmethodID=falseHow to test the change?:
Running renaissance akka benchmark with NMT, exams
jmethodIDsites:Mainline (without this PR)
With
force_jmethodID=falseFor Datadog employees:
credentials of any kind, I've requested a review from
@DataDog/security-design-and-guidance.Unsure? Have a question? Request a review!