fix(nativemem): skip building DWARF unwind tables when cstack mode doesn't need them - #755
fix(nativemem): skip building DWARF unwind tables when cstack mode doesn't need them#755rkennke wants to merge 1 commit into
Conversation
…esn't need them ElfParser::parseDwarfInfo() previously built SFrame/DWARF unwind tables for every parsed native library unconditionally, even though they are read only by StackWalker::walkDwarf() under CSTACK_DWARF. Measured cost: 4.52 MiB across 22 allocations at 150,000 loaded classes, even in the common case where the default cstack mode resolves to CSTACK_VM (whenever HotSpot VMStructs are available) rather than CSTACK_DWARF. Gating this correctly needs more than comparing cstackMode() to CSTACK_DWARF: Profiler::_cstack's constructor default is CSTACK_NO, a real user-selectable mode, not a distinguishable "not yet resolved" sentinel. Since libraries are parsed exactly once and never re-parsed (Symbols::parseLibraries tracks _parsed_inodes) -- including the handful parsed at JVMTI Agent_OnLoad, before the later VM::VMInit()-triggered Profiler::start() call resolves cstack mode -- a naive comparison would permanently and silently skip building the table for those libraries (libjvm.so among them) even in an actual CSTACK_DWARF session, degrading native-frame unwind quality with no visible error. Adds Profiler::_cstack_resolved, set once immediately after start()'s resolution logic and before any sampling begins, to disambiguate "not yet decided" from "decided, and it's CSTACK_NO". parseDwarfInfo() only skips once cstackResolved() is true and the resolved mode isn't CSTACK_DWARF. Verified: cstack=dwarf correctly builds the full table (NM_NATIVE_SYMBOLS 13.03 MiB, matching the pre-gate value exactly); stackWalker_ut, libraries_ut, and codeCache_ut (release+debug) all pass. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Closing without merging per the honest cost/benefit note above: the corrected fix is real and tested, but the measured benefit on the only workload we tested it against is ~0.07 MiB, not worth the added complexity right now. Left open as a reference/starting point for whoever has a workload where dynamically-loaded native libraries during active non-DWARF profiling actually matter. |
![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: b436f83ec8
ℹ️ 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".
| if (Profiler::instance()->cstackResolved() && | ||
| Profiler::instance()->cstackMode() != CSTACK_DWARF) { | ||
| return; |
There was a problem hiding this comment.
When an initial recording uses cstack=vm, fp, or no and a library is loaded during that session, this return creates its CodeCache without an unwind table, while Symbols::_parsed_inodes prevents that library from ever being parsed again. A later supported stop/start cycle with cstack=dwarf therefore walks frames from that library using the fallback descriptor instead of its SFrame/DWARF data, silently producing incorrect or truncated native stacks. Retain the unwind data or explicitly populate missing tables when transitioning to DWARF.
Useful? React with 👍 / 👎.
Scan-Build Report
Bug Summary
Reports
|
||||||||||||||||||||||||||||||||||||
CI Test ResultsRun: #32883840993 | Commit:
Status Overview
Legend: ✅ passed | ❌ failed | ⚪ skipped | 🚫 cancelled Failed Testsglibc-aarch64/debug / 8-j9Job: View logs No detailed failure information available. Check the job logs. Summary: Total: 32 | Passed: 31 | Failed: 1 Updated: 2026-08-25 18:51:45 UTC |
…fix, the bug it caught, and why it's closed The gate was implemented and initially measured as a ~4.52 MiB win, but that measurement was against a buggy version (Profiler::_cstack's constructor default is CSTACK_NO, a real mode, not a distinguishable "unresolved" sentinel -- a naive gate would have permanently and silently broken DWARF unwinding for libjvm.so itself in an actual CSTACK_DWARF session). Fixed properly with a dedicated _cstack_resolved flag, then re-measured: the real benefit on our benchmark is ~0.07 MiB, not 4.52 MiB, because almost the entire native-library set for that workload loads before Profiler::start() resolves cstack, where the gate must conservatively always build. Submitted as #755 and closed without merging -- the fix is correct and tested, but not worth the added complexity given the measured benefit, though it may help workloads that dlopen native libraries while actively profiling (unmeasured here). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
1 participant
Summary
ElfParser::parseDwarfInfo()builds SFrame/DWARF unwind tables for every parsed native library unconditionally, even though they're read only byStackWalker::walkDwarf()underCSTACK_DWARF. Measured at 4.52 MiB / 22 allocations at 150,000 loaded classes, paid even when the resolved mode isCSTACK_VM(the common default whenever HotSpot VMStructs are available).CSTACK_DWARF. Getting "certain" right requires a dedicatedProfiler::_cstack_resolvedflag, not just comparingcstackMode()—_cstack's constructor default isCSTACK_NO, a real user-selectable mode, not a distinguishable "not yet decided" sentinel. Since libraries are parsed exactly once (Symbols::parseLibrariestracks_parsed_inodes), including the handful parsed at JVMTIAgent_OnLoadbeforeProfiler::start()ever resolves cstack mode, a naive check would permanently and silently skip building the table for those libraries (libjvm.soamong them) even in an actualCSTACK_DWARFsession — degrading native-frame unwind quality with no visible error._cstack_resolvedis set once, right afterstart()'s resolution logic, before any sampling begins.Honest note on measured impact — why this is closed rather than pursued further right now
The corrected fix is safe (verified
cstack=dwarfstill builds the full table, no regression). But re-measuring after fixing the above turned up something worth flagging: on the benchmark workload used to find this (classesmode, 150,000 loaded classes), the real savings are only ~0.07 MiB, not the ~4.52 MiB originally measured with the buggy gate. Almost the entire native-library set for that workload loads beforeProfiler::start()runs, where the gate must conservatively always build (the eventual mode isn't knowable yet). Only a handful of JDK-internal libraries that load lazily after start (libmanagement.so,libnet.so,libnio.so, etc.) actually benefit.This fix should have more value for workloads that
dlopennative libraries while actively profiling in a non-DWARF mode (JNI-heavy applications, dynamically-attached native agents) — but that scenario isn't covered by any workload we've measured, so the benefit there is currently unquantified, not just small. Given the added complexity (a newProfilerfield threaded through a lifecycle-ordering invariant) isn't clearly justified by a measured benefit at this point, closing this without merging. Leaving it open here as reference in case someone has a workload where this matters and wants to pick it up — the fix itself is complete and tested.Test plan
stackWalker_ut,libraries_ut,codeCache_ut(release+debug) all passcstack=dwarfbuilds the full table (NM_NATIVE_SYMBOLS= 13.03 MiB, matching the ungated value exactly — no regression)cstack=fpcorrectly skips the ~0.07 MiB that's actually skippable for that workload🤖 Generated with Claude Code