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 28 commits into
Conversation
This comment has been minimized.
This comment has been minimized.
CI Test ResultsRun: #27226908405 | Commit:
Status Overview
Legend: ✅ passed | ❌ failed | ⚪ skipped | 🚫 cancelled Summary: Total: 32 | Passed: 32 | Failed: 0 Updated: 2026-06-09 18:58:57 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".
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".
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!
| 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
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.
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".
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".
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".
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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4702a6289d
ℹ️ 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 (method_id != NULL) { | ||
| fillFrame(frames[depth++], FRAME_JIT_COMPILED, 0, method_id); | ||
| } | ||
| fillFrame(frames[depth++], FRAME_JIT_COMPILED, 0, method_id, method); |
There was a problem hiding this comment.
When features.comp_task is enabled and a marked compiler-entry frame is sampled while the compiler thread has no current CompileTask, vm_thread->compiledMethod() returns NULL; this now still calls the new overload with both method_id and method null. That overload falls into the raw-pointer path, which asserts in debug builds and records a raw null method in release builds instead of skipping the pseudo-frame as before, so idle compiler-thread samples can abort or produce spurious unknown Java frames.
Useful? React with 👍 / 👎.
rkennke
left a comment
rkennke
left a comment
There was a problem hiding this comment.
This is a great improvement!
Here comes my first pass of (manual) review. It's mostly minor stuff.
| msg = "jstackdepth must be > 0"; | ||
| } | ||
|
|
||
| CASE("fjmethodid") |
There was a problem hiding this comment.
I find it unintuitive/confusing that in order to turn the new feature on, we have to set the flag to false.
There was a problem hiding this comment.
I don't like to name, what do you suggest?
There was a problem hiding this comment.
Yeah I don't know either, it seems hard. I could only come up with dont-preload-methodIds or no-preload-methodIds, but it doesn't seem very much better. I leave that up to you.
| HotspotSupport::initialize(jni); | ||
| } | ||
|
|
||
| loadAllMethodIDs(jvmti, jni); |
There was a problem hiding this comment.
It is slightly confusing that this call says 'load all method IDs, even in case of HotSpot', even though we don't actually load all method IDs when running HotSpot - but this is only decided later.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 39149aff27
ℹ️ 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".
| method_id = jni->GetMethodID(clz, method_name, method_signature); | ||
| if (method_id == nullptr) { | ||
| jni->ExceptionClear(); | ||
| method_id = jni->GetStaticMethodID(clz, method_name, method_signature); |
There was a problem hiding this comment.
When fjmethodid=false skips preloading for system/application classes, samples taken in a class initializer reach this raw-Method* resolver with the VM name <clinit>. JNI has no jmethodID lookup for class initializers via GetMethodID or GetStaticMethodID, so both calls leave method_id null and Lookup::resolveMethod() serializes those Java frames as unknown even though the VM method metadata is available. This affects cstack=vm/raw-pointer recordings that sample application or JDK class initialization paths.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ed1c0471bb
ℹ️ 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::isHidden(jint modifiers) { | ||
| // 0x1400 = ACC_SYNTHETIC(0x1000) | ACC_BRIDGE(0x0040) | ||
| return modifiers == 0 || (modifiers & 0x1040); | ||
| } |
There was a problem hiding this comment.
When fjmethodid=false, HotspotSupport::loadMethodIDsImpl() uses this helper to decide whether a system/application-loader class can skip GetClassMethods. JVMTI GetClassModifiers returns 0 for ordinary package-private classes, so those non-hidden classes are treated as hidden and fall through to JVMSupport::loadMethodIDsImpl(), preloading jmethodIDs for many package-private JDK/app classes even though the option is meant to avoid that memory cost for normal system-loader classes.
Useful? React with 👍 / 👎.
| if (!isHiddenClass(jvmti, klass) && | ||
| jvmti->GetClassLoader(klass, &cl) == JVMTI_ERROR_NONE && | ||
| isSystemClassLoader(jni, cl)) { |
There was a problem hiding this comment.
When fjmethodid=false is used at JVM startup, VM::initProfilerBridge() enables JVMTI_EVENT_CLASS_PREPARE before VMInit calls JVMSupport::initialize(), so early platform/application classes can reach this check while JAVA_PLATFORM_CLASSLOADER and JAVA_APPLICATION_CLASSLOADER are still null. In that window isSystemClassLoader() only matches bootstrap classes, so platform/app classes fall through to GetClassMethods and are preloaded anyway; initialize these globals before enabling ClassPrepare or defer the skip until they are ready.
Useful? React with 👍 / 👎.
rkennke
left a comment
rkennke
left a comment
There was a problem hiding this comment.
Thanks for making the changes. Some old comments are still un-addressed, and I also have some new comments.
| if (method_id != NULL) { | ||
| fillFrame(frames[depth++], FRAME_JIT_COMPILED, 0, method_id); | ||
| } | ||
| fillFrame(frames[depth++], FRAME_JIT_COMPILED, 0, method_id, method); |
There was a problem hiding this comment.
There is a logic bug here, and Codex already pointed that out.
| static name * load_then_cast(const void* ptr) { \ | ||
| assert(ptr != nullptr); \ | ||
| return cast(*(const void**)ptr); } | ||
| return cast_or_null(*(const void**)ptr); } |
There was a problem hiding this comment.
The old code asserted ptr to be != nullptr. The new code blindly dereferences ptr. I would either put the assert back or, if pr == nullptr is now a valid input, guard against ptr == nullptr.
| method_id = JVMSupport::resolve(frame.method); | ||
| } | ||
|
|
||
| /** |
There was a problem hiding this comment.
I think commented-out code should be removed.
| static jobject JAVA_APPLICATION_CLASSLOADER = nullptr; | ||
|
|
||
| void HotspotSupport::initialize(JNIEnv* jni) { | ||
| if (JAVA_PLATFORM_CLASSLOADER != nullptr || JAVA_APPLICATION_CLASSLOADER != nullptr) { |
There was a problem hiding this comment.
This assumes that:
- initialize() is not called concurrently by different threads
- initialization finds both class-loaders, otherwise re-init would override already-initialized field
Are both assumptions reasonable?
| } | ||
|
|
||
| bool JVMSupport::isHidden(jint modifiers) { | ||
| // 0x1400 = ACC_SYNTHETIC(0x1000) | ACC_BRIDGE(0x0040) |
| strstr(signature, "$$Lambda.") != nullptr || | ||
| strstr(signature, "$$Lambda$") != nullptr || | ||
| strstr(signature, ".lambda$") != nullptr || | ||
| strncmp(signature, FFM_PREFIX, strlen(FFM_PREFIX)) == 0; |
There was a problem hiding this comment.
you can use sizeof(FFM_PREFIX) - 1 to avoid the runtime call, because the string is a constexpr.
| jni->IsSameObject(cl, JAVA_APPLICATION_CLASSLOADER); // application class loader (system class loader) | ||
| } | ||
|
|
||
| bool isHiddenClass(jvmtiEnv *jvmti, jclass clazz) { |
There was a problem hiding this comment.
I think this can/should be static.
| * SPDX-License-Identifier: Apache-2.0 | ||
| */ | ||
|
|
||
| #include "flightRecorder.h" |
| VMMethod* vm_method = VMMethod::load_then_cast((const void*)id); | ||
| return vm_method == NULL || vm_method->id() == NULL; | ||
| // jmethod ID == nullptr, means the value is not yet populated | ||
| return vm_method == NULL || vm_method->id() == JMETHODID_NOT_WALKABLE; |
There was a problem hiding this comment.
What about the case vm_method->id() == NULL? (and why are we using NULL here, but nullptr elsewhere?)
| } | ||
|
|
||
| // This method only resolves methods that are loaded by system class loaders | ||
| jmethodID HotspotSupport::resolve(const void* method) { |
There was a problem hiding this comment.
This must only ever be called outside a signal-handler-context. If possible, this should be asserted, or otherwise at least documented.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b5a898f44d
ℹ️ 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 (VM::isHotspot() && HotspotSupport::shouldPreloadJmethodIDs()) { | ||
| return HotspotSupport::loadMethodIDsImpl(jvmti, jni, klass); | ||
| } else { | ||
| return loadMethodIDsImpl(jvmti, jni, klass); |
There was a problem hiding this comment.
For cstack=fp/dwarf/lbr/no, shouldPreloadJmethodIDs() returns true, so this branch calls HotspotSupport::loadMethodIDsImpl, which skips GetClassMethods for normal bootstrap/platform/application classes. Those stack modes still use the ASGCT path (cstack < CSTACK_VM), which depends on pre-created jmethodIDs, so normal Java frames in those recordings can become missing/unknown; the same inversion makes explicit cstack=vm fall into the unconditional preloader instead of the raw Method* path.
Useful? React with 👍 / 👎.
3 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!