RFR: 8354520: IGV: dump contextual information [v5]

Mon May 5 11:53:11 UTC 2025

On Wed, 30 Apr 2025 14:14:53 GMT, Emanuel Peter <epeter at openjdk.org> wrote:

>> Roberto Castañeda Lozano has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Document workaround for lldb issue
>
> 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)

Done (commit 6b8e659).

> 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) {

Done (commit 6b8e659).

> 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)

Done (commit 6b8e659).

> 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)

Done (commit 6b8e659).

> 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)

Done (commit 6b8e659).

> 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.

Done (commit cb781a95).

> 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 :)

Thanks, I went with `should_skip_frame` (commit 1210180b).

> 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?

Thanks, I went with `should_end_stack_walk`, and added a comment as requested (commit 1210180b).

> 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.

Thanks, the underscore was just a leftover from copying some code from `NativeStackPrinter::print_stack_from_frame()`. I just inlined `Thread::current_or_null()` into its only use, which I think is even clearer (commit `13604b75`).

> 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.

Done, thanks (commit 9cd331d2).

> 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?

I refactored this code in commit 6fa328e0 to pass the initial frame by reference.

> 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?

Done (commit 3c3108b8), thanks.

> 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)

Done (commit 6b8e659).

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/24724#discussion_r2073286020
PR Review Comment: https://git.openjdk.org/jdk/pull/24724#discussion_r2073286083
PR Review Comment: https://git.openjdk.org/jdk/pull/24724#discussion_r2073286175
PR Review Comment: https://git.openjdk.org/jdk/pull/24724#discussion_r2073286248
PR Review Comment: https://git.openjdk.org/jdk/pull/24724#discussion_r2073286345
PR Review Comment: https://git.openjdk.org/jdk/pull/24724#discussion_r2073287440
PR Review Comment: https://git.openjdk.org/jdk/pull/24724#discussion_r2073288991
PR Review Comment: https://git.openjdk.org/jdk/pull/24724#discussion_r2073290430
PR Review Comment: https://git.openjdk.org/jdk/pull/24724#discussion_r2073293032
PR Review Comment: https://git.openjdk.org/jdk/pull/24724#discussion_r2073293540
PR Review Comment: https://git.openjdk.org/jdk/pull/24724#discussion_r2073296132
PR Review Comment: https://git.openjdk.org/jdk/pull/24724#discussion_r2073297968
PR Review Comment: https://git.openjdk.org/jdk/pull/24724#discussion_r2073286585