RFR: 8354520: IGV: dump contextual information [v5]
Emanuel Peter
epeter at openjdk.org
Wed Apr 30 14:57:56 UTC 2025
On Wed, 30 Apr 2025 12:10:00 GMT, Roberto Castañeda Lozano <rcastanedalo at openjdk.org> wrote:
>> This changeset extends the IGV graph dumps with additional properties that ease tracing the dumps back to the context in which they were produced. The changeset dumps, for every compilation, the following additional properties:
>>
>> - JVM arguments
>> - platform information
>> - JVM version information
>> - date and time
>> - process ID
>> - (compiler) thread ID
>>
>> 
>>
>> Additionally, the changeset produces and dumps the C2 stack trace from which each graph is dumped:
>>
>> 
>>
>> This should be particularly useful in an interactive context, where the user steps through C2 code using a debugger and dumps graphs at different points. To produce a stack trace in this context, the usual debugger-entry C2 functions (`igv_print`, `igv_append`, `Node::dump_bfs`, ...) are extended with extra arguments to specify the stack handling registers (stack pointer, frame pointer, and program counter):
>>
>> 
>>
>> The inconvenience of manually specifying the stack handling registers can be addressed by hiding them in debugger user-defined commands, e.g.:
>>
>>
>> define igv
>> p igv_print(true, $sp, $fp, $pc)
>> end
>>
>> define igv_node
>> p find_node($arg0)->dump_bfs(0, 0, "!", $sp, $fp, $pc)
>> end
>>
>>
>> Thanks to @TobiHartmann for providing useful feedback!
>>
>> #### Testing
>>
>> - tier1 (windows-x64, linux-x64, linux-aarch64, macosx-x64, and macosx-aarch64; release and debug mode).
>> - Tested interactive usage manually via `gdb` and `rr` on linux-x64.
>> - Tested automatically that dumping thousands of graphs does not trigger any assertion failure.
>
> Roberto Castañeda Lozano has updated the pull request incrementally with one additional commit since the last revision:
>
> Document workaround for lldb issue
Nice work @robcasloz !
I just left a few suggestions below, but I think they are basically all nits / optional :)
src/hotspot/share/opto/compile.cpp line 5207:
> 5205:
> 5206: // Called from debugger. Prints method to the default file with the default phase name.
> 5207: // This works regardless of any Ideal Graph Visualizer flags set or not.
Suggestion:
// This works regardless of any Ideal Graph Visualizer flags set or not.
// Use in debugger (gdb / rr): p igv_print($sp, $fp, $pc)
src/hotspot/share/opto/compile.cpp line 5222:
> 5220: // the network flags for the Ideal Graph Visualizer, or to the default file depending on the 'network' argument.
> 5221: // This works regardless of any Ideal Graph Visualizer flags set or not.
> 5222: void igv_print(bool network, void* sp, void* fp, void* pc) {
Suggestion:
// Use in debugger (gdb / rr): p igv_print(true, $sp, $fp, $pc)
void igv_print(bool network, void* sp, void* fp, void* pc) {
src/hotspot/share/opto/compile.cpp line 5231:
> 5229: }
> 5230:
> 5231: // Same as igv_print(bool network) above but with a specified phase name.
Suggestion:
// Same as igv_print(bool network, void* sp, void* fp, void* pc) above but with a specified phase name.
// Use in debugger (gdb / rr): p igv_print(true, "MyPhase", $sp, $fp, $pc)
src/hotspot/share/opto/compile.cpp line 5248:
> 5246: // Called from debugger, especially when replaying a trace in which the program state cannot be altered like with rr replay.
> 5247: // A method is appended to an existing default file with the default phase name. This means that igv_append() must follow
> 5248: // an earlier igv_print(*) call which sets up the file. This works regardless of any Ideal Graph Visualizer flags set or not.
Suggestion:
// an earlier igv_print(*) call which sets up the file. This works regardless of any Ideal Graph Visualizer flags set or not.
// Use in debugger (gdb / rr): p igv_append($sp, $fp, $pc)
src/hotspot/share/opto/compile.cpp line 5254:
> 5252: }
> 5253:
> 5254: // Same as igv_append() above but with a specified phase name.
Suggestion:
// Same as igv_append(void* sp, void* fp, void* pc) above but with a specified phase name.
// Use in debugger (gdb / rr): p igv_append("MyPhase", $sp, $fp, $pc)
src/hotspot/share/opto/idealGraphPrinter.cpp line 380:
> 378: print_prop(COMPILATION_PROCESS_ID_PROPERTY, os::current_process_id());
> 379: print_prop(COMPILATION_THREAD_ID_PROPERTY, os::current_thread_id());
> 380:
What about CPU features? Could be nice to know if we have `avx2` or `asimd`, etc.
src/hotspot/share/opto/idealGraphPrinter.cpp line 907:
> 905: }
> 906:
> 907: static bool skip_frame(const char* name) {
Nit: the name suggests that this is "skipping a frame". But you are asking if we "should skip the frame". So I would recommend a name like `is_skip_frame`, `must_skip_frame`, `should_skip_frame` or alike.
You could even invert the condition, and make the condition positive: `can_print_stack_frame`.
Totally optional, up to you :)
src/hotspot/share/opto/idealGraphPrinter.cpp line 917:
> 915: static bool stop_frame_walk(const char* name) {
> 916: return strstr(name, "C2Compiler::compile_method") != nullptr;
> 917: }
Nit: You could write `must_stop_frame_walk`. Btw, it could be nice to have a comment to explain why this is the condition to stop on. Maybe that comment could then turn into an even better method name?
src/hotspot/share/opto/idealGraphPrinter.cpp line 921:
> 919: void IdealGraphPrinter::print_stack(frame fr, outputStream* graph_name) {
> 920: char buf[O_BUFLEN];
> 921: Thread* _current = Thread::current_or_null();
Is this a local variable? If so, I would drop the underscore. It generally suggests that it is a field, right?
It would also not hurt to call it `current_thread` for clarity.
src/hotspot/share/opto/idealGraphPrinter.cpp line 924:
> 922: int count = 0;
> 923: int frame = 0;
> 924: while (count++ < StackPrintLimit && fr.pc() != nullptr) {
Could this be formulated as a `for` loop?
Suggestion:
for (int count = 0; count < StackPrintLimit && fr.pc() != nullptr; count++) {
Just an idea, totally optional.
src/hotspot/share/opto/idealGraphPrinter.cpp line 967:
> 965: if (!_current_method || !_should_send_method || node == nullptr) return;
> 966:
> 967: frame current = fr == nullptr ? os::current_frame() : *fr;
Suggestion:
frame current_frame = fr == nullptr ? os::current_frame() : *fr;
Just to make sure there is no confusion with `_current_method`.
Optional, up to you.
src/hotspot/share/opto/idealGraphPrinter.hpp line 122:
> 120: // graph_name == nullptr) or the graph name based on the highest C2 frame (if
> 121: // graph_name != nullptr).
> 122: void print_stack(frame fr, outputStream* graph_name);
Are you passing this `frame` by value on purpose?
src/hotspot/share/opto/node.cpp line 1799:
> 1797: const char* _options;
> 1798: outputStream* _output;
> 1799: frame* _frame;
Could this be `const`, like the other pointers?
src/hotspot/share/opto/node.cpp line 2428:
> 2426: }
> 2427:
> 2428: // Call this from debugger, with stack handling register arguments for IGV dumps.
Suggestion:
// Call this from debugger, with stack handling register arguments for IGV dumps.
// Example: p find_node(741)->dump_bfs(7, find_node(741), "c+A!", $sp, $fp, $pc)
-------------
Marked as reviewed by epeter (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/24724#pullrequestreview-2807226660
PR Review Comment: https://git.openjdk.org/jdk/pull/24724#discussion_r2068758515
PR Review Comment: https://git.openjdk.org/jdk/pull/24724#discussion_r2068761567
PR Review Comment: https://git.openjdk.org/jdk/pull/24724#discussion_r2068763703
PR Review Comment: https://git.openjdk.org/jdk/pull/24724#discussion_r2068766019
PR Review Comment: https://git.openjdk.org/jdk/pull/24724#discussion_r2068767093
PR Review Comment: https://git.openjdk.org/jdk/pull/24724#discussion_r2068770939
PR Review Comment: https://git.openjdk.org/jdk/pull/24724#discussion_r2068779813
PR Review Comment: https://git.openjdk.org/jdk/pull/24724#discussion_r2068784104
PR Review Comment: https://git.openjdk.org/jdk/pull/24724#discussion_r2068787022
PR Review Comment: https://git.openjdk.org/jdk/pull/24724#discussion_r2068793710
PR Review Comment: https://git.openjdk.org/jdk/pull/24724#discussion_r2068804402
PR Review Comment: https://git.openjdk.org/jdk/pull/24724#discussion_r2068817524
PR Review Comment: https://git.openjdk.org/jdk/pull/24724#discussion_r2068840383
PR Review Comment: https://git.openjdk.org/jdk/pull/24724#discussion_r2068846400
More information about the hotspot-compiler-dev
mailing list