RFR: 8261336: IGV: enhance default filters

[Christian Hagedorn]

On Wed, 10 Feb 2021 10:00:00 GMT, Roberto Castañeda Lozano <rcastanedalo at openjdk.org> wrote:

> Redesign the filters shown by default in the "Filters" window:
> 
> - Add filters to color the graph by node category and execution frequency (if applicable), and to hide subgraphs or edges only by _category_. The category of a node can be one of {`data`, `memory`, `control`, `mixed`, `other`}, and is solely determined by its type. `mixed` nodes are those with a tuple type that has different categories, such as `CallStaticJavaNode`. The category of an edge is that of its source node.
> 
> - Instrument C2 to include the category and estimated execution frequency (if available) of each node in the graph dumps produced by `-XX:PrintIdealGraphLevel=N` (only in debug builds).
> 
> - Remove filters which depend on properties never emitted by C2 (e.g. 'Remove State') or which appear to be unused ('C2 Matcher Flags Coloring' and 'C2 Register Coloring'). Also remove the subsumed 'C2 Basic Coloring' filter. 
> 
> - Merge 'C2 Remove Filter' and 'C2 Structural' into a single filter with a clearer name ('Simplify graph').
> 
> ### Screenshots
> 
> "Filters" window before (left) and after (right) the proposed change:
> ![filters-window](https://user-images.githubusercontent.com/8792647/107749859-7a664780-6d1b-11eb-84ba-fd43e13abd0e.png)
> Default color scheme before (left) and after (right) the proposed change:
> ![color-scheme](https://user-images.githubusercontent.com/8792647/107517355-f3479100-6bad-11eb-9b0b-a71c18961dd8.png)
> Examples of the new 'Color by execution frequency' filter:
> ![color-by-frequency-2](https://user-images.githubusercontent.com/8792647/107518492-5980e380-6baf-11eb-9e01-992b211d06e3.png)
> ![color-by-frequency-1](https://user-images.githubusercontent.com/8792647/107518477-54bc2f80-6baf-11eb-8c7b-7eb7c1d85cf7.png)
> Example of the new 'Hide X subgraph'' filters, where only the data subgraph is shown:
> ![hide-all-but-data-subgraphs](https://user-images.githubusercontent.com/8792647/107750398-42133900-6d1c-11eb-8b27-264086b32bea.png)
> Example of the new 'Hide X edges' filters, where all nodes remain in their position but only the memory edges are shown:
> ![hide-all-but-memory-edges](https://user-images.githubusercontent.com/8792647/107751137-49871200-6d1d-11eb-9df0-79747bf8d16e.png)
> 
> 
> Tested IGV manually on a few graphs. Tested C2 instrumentation by running `hs-tier1` with `-Xbatch -XX:PrintIdealGraphLevel=3 -XX:PrintIdealGraphFile=graph.xml` on windows-x64, linux-x64, linux-aarch64, and macosx-x64 (all debug).

Otherwise, very nice improvement! I applied the patch and played around with it in the IGV. Everything seems to work as expected. I like the new default filters and the new colors for the new categories. That makes it much more useful.

src/utils/IdealGraphVisualizer/ServerCompiler/src/com/sun/hotspot/igv/servercompiler/filters/onlyControlFlow.filter line 23:

> 21:   )
> 22: );
> 23: f.addRule(new RemoveFilter.RemoveRule(new MatcherSelector(new Properties.RegexpPropertyMatcher("name", "Phi|CreateEx|Cast.*|Load.|Store."))));

I tried the filter out on a graph and noticed that there are still some data and memory nodes shown. How about utilizing the newly added categories? I played around a bit and this seems to work quite nicely. What do you think?

var f = new RemoveFilter("Show only control flow");
f.addRule(
  new RemoveFilter.RemoveRule(
    new OrSelector(
      new InvertSelector(
        new MatcherSelector(
          new Properties.RegexpPropertyMatcher("category", "control|mixed|other")
        )
      ),
      new MatcherSelector(
        new Properties.StringPropertyMatcher("type", "abIO")
      )
    ), false
  )
);
f.addRule(new RemoveFilter.RemoveRule(new MatcherSelector(new Properties.RegexpPropertyMatcher("name", "Phi|Root|Con"))));
f.apply(graph);

src/hotspot/share/opto/idealGraphPrinter.hpp line 95:

> 93:   bool _traverse_outs;
> 94:   Compile *C;
> 95:   double max_freq;

Fields should have a leading underscore.

src/hotspot/share/opto/type.cpp line 1120:

> 1118: Type::CATEGORY Type::category() const {
> 1119:   const TypeTuple* tuple;
> 1120:   switch (base()) {

Might be more readable if switch cases are indented.

src/hotspot/share/opto/type.hpp line 374:

> 372:     CatControl,
> 373:     CatOther,   // {Type::Top, Type::Abio, Type::Bottom}.
> 374:     CatUndef    // {Type::Bad, Type::lastype}, for completeness.

Is "Cat" required when the enum is already called "CATEGORY"?

src/hotspot/share/opto/idealGraphPrinter.cpp line 394:

> 392:     }
> 393: 
> 394:     switch (t->category()) {

Might be more readable if switch cases are indented.

src/hotspot/share/opto/type.cpp line 1178:

> 1176:   }
> 1177:   assert(false, "unmatched base type");
> 1178:   return CatUndef;

You could add that to an explicit default case to the above switch statement to make it more clear.

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

Changes requested by chagedorn (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/2499