RFR: 8344122: IGV: Extend c2 IdealGraphPrinter to send subgraphs to IGV

[Roberto Castañeda Lozano]

On Wed, 13 Nov 2024 14:41:24 GMT, Tobias Holenstein <tholenstein at openjdk.org> wrote:

> IGV XML already support to define which graphs are visible when opened. Extend the IdealGraphPrinter::print... in C2 to define which nodes should be visible in IGV when sent over the network
> 
> ### Add a new option "!" to dump_bfs
> The option ! send the printed nodes of dump_bfs to IGV and shows them 
> 
> p find_node(0)->dump_bfs(1,0,"dcmxo+!")
> 
> dist dump
> ---------------------------------------------
>    1  51  Return  === 46 6 47 8 9 returns 39  [[ 0 ]] 
>    0   0  Root  === 0 51  [[ 0 1 3 26 ]] 
> Method printed over network stream to IGV
> 
> 
> <img width="668" alt="dump" src="https://github.com/user-attachments/assets/d476e8b0-c444-4cd3-b40d-4a8f35caba83">

Great improvement, thanks Toby! Please add a line to `PrintBFS::print_options_help` documenting the new option, something like: `  _output->print_cr("      !: show nodes on IGV (sent over network stream)");`. Otherwise, I just have a couple of minor suggestions and comments.

As a follow-up improvement, it would be great if we could similarly extend `igv_print(bool network)`, when `network == true`, to tell IGV to automatically open and focus the graph sent from the debugger.

src/hotspot/share/opto/compile.cpp line 5288:

> 5286:   ResourceMark rm;
> 5287:   GrowableArray<const Node*> empty_list;
> 5288:   igv_print_graph_to_network(phase_name, (Node *) C->root(), empty_list);

Suggestion:

  igv_print_graph_to_network(phase_name, (Node*) C->root(), empty_list);

src/hotspot/share/opto/compile.cpp line 5291:

> 5289: }
> 5290: 
> 5291: void Compile::igv_print_graph_to_network(const char* name, Node* node, GrowableArray<const Node*> & visible_nodes) {

Suggestion:

void Compile::igv_print_graph_to_network(const char* name, Node* node, GrowableArray<const Node*>& visible_nodes) {

src/hotspot/share/opto/node.cpp line 2054:

> 2052:     if (_print_igv) {
> 2053:       Compile* C = Compile::current();
> 2054:       if (C->should_print_igv(0)) {

I guess the reason to call `should_print_igv` here is to initialize `Compile::_igv_printer` if necessary. Would it be possible to factor out the initialization part https://github.com/openjdk/jdk/blob/2145ace384137b1c028a68dc34a8800577c7a43e/src/hotspot/share/opto/compile.cpp#L5214-L5217 into a separate function that only checks if `_igv_printer` is `nullptr` and, if so, initializes it, and only call that function from here?

src/hotspot/share/opto/node.cpp line 2055:

> 2053:       Compile* C = Compile::current();
> 2054:       if (C->should_print_igv(0)) {
> 2055:         C->igv_print_graph_to_network("PrintBFS", (Node*) Compile::current()->root(), _print_list);

Suggestion:

        C->igv_print_graph_to_network("PrintBFS", (Node*) C->root(), _print_list);

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

Changes requested by rcastanedalo (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/22076#pullrequestreview-2435915234
PR Review Comment: https://git.openjdk.org/jdk/pull/22076#discussion_r1842118240
PR Review Comment: https://git.openjdk.org/jdk/pull/22076#discussion_r1842121007
PR Review Comment: https://git.openjdk.org/jdk/pull/22076#discussion_r1842136714
PR Review Comment: https://git.openjdk.org/jdk/pull/22076#discussion_r1842130090