RFR: 8261336: IGV: enhance default filters

classic Classic list List threaded Threaded
16 messages Options
Reply | Threaded
Open this post in threaded view
|

RFR: 8261336: IGV: enhance default filters

Roberto Castañeda Lozano-2
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).

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

Commit messages:
 - Use 'subgraph' instead of 'nodes' in filter names, for clarity
 - Add filters to hide edges by category
 - Update year in headers
 - Adjust frequency precision
 - Improve color scale in 'Color by execution frequency' filter
 - Add filter to color nodes by estimated execution frequency
 - Lift simplification filter
 - Merge simplification filters into a single, more intuitive one
 - Add filters to hide nodes by category
 - Reorder filters to preserve the behavior of 'Show control flow only'
 - ... and 4 more: https://git.openjdk.java.net/jdk/compare/db0ca2b9...3ed1e3de

Changes: https://git.openjdk.java.net/jdk/pull/2499/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=2499&range=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8261336
  Stats: 359 lines in 25 files changed: 284 ins; 40 del; 35 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2499.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2499/head:pull/2499

PR: https://git.openjdk.java.net/jdk/pull/2499
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8261336: IGV: enhance default filters

Vladimir Ivanov-2
On Wed, 10 Feb 2021 10:00:00 GMT, Roberto Castañeda Lozano <[hidden email]> 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).

Very nice!

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

Marked as reviewed by vlivanov (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/2499
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8261336: IGV: enhance default filters

Roberto Castañeda Lozano-2
On Fri, 12 Feb 2021 10:34:59 GMT, Vladimir Ivanov <[hidden email]> wrote:

> Very nice!

Thanks Vladimir!

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

PR: https://git.openjdk.java.net/jdk/pull/2499
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8261336: IGV: enhance default filters

Christian Hagedorn-3
In reply to this post by Roberto Castañeda Lozano-2
On Wed, 10 Feb 2021 10:00:00 GMT, Roberto Castañeda Lozano <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8261336: IGV: enhance default filters

Roberto Castañeda Lozano-2
On Fri, 12 Feb 2021 12:48:50 GMT, Christian Hagedorn <[hidden email]> wrote:

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

Thanks for reviewing Christian! I will have a look at your comments on Monday.

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

PR: https://git.openjdk.java.net/jdk/pull/2499
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8261336: IGV: enhance default filters [v2]

Roberto Castañeda Lozano-2
In reply to this post by Roberto Castañeda Lozano-2
> 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).

Roberto Castañeda Lozano has updated the pull request incrementally with five additional commits since the last revision:

 - Rewrite 'Show control flow only' filter using categories
 - Add leading underscore field
 - Move assertion to a default switch case
 - Indent switch statements
 - Use a scoped enum for type categories (as per the HotSpot style guide)

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

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2499/files
  - new: https://git.openjdk.java.net/jdk/pull/2499/files/3ed1e3de..adcb9181

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2499&range=01
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2499&range=00-01

  Stats: 105 lines in 5 files changed: 9 ins; 5 del; 91 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2499.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2499/head:pull/2499

PR: https://git.openjdk.java.net/jdk/pull/2499
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8261336: IGV: enhance default filters [v2]

Roberto Castañeda Lozano-2
In reply to this post by Christian Hagedorn-3
On Fri, 12 Feb 2021 12:14:53 GMT, Christian Hagedorn <[hidden email]> wrote:

>> Roberto Castañeda Lozano has updated the pull request incrementally with five additional commits since the last revision:
>>
>>  - Rewrite 'Show control flow only' filter using categories
>>  - Add leading underscore field
>>  - Move assertion to a default switch case
>>  - Indent switch statements
>>  - Use a scoped enum for type categories (as per the HotSpot style guide)
>
> 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.

Done.

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

Done.

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

PR: https://git.openjdk.java.net/jdk/pull/2499
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8261336: IGV: enhance default filters [v2]

Roberto Castañeda Lozano-2
In reply to this post by Christian Hagedorn-3
On Fri, 12 Feb 2021 12:18:35 GMT, Christian Hagedorn <[hidden email]> wrote:

>> Roberto Castañeda Lozano has updated the pull request incrementally with five additional commits since the last revision:
>>
>>  - Rewrite 'Show control flow only' filter using categories
>>  - Add leading underscore field
>>  - Move assertion to a default switch case
>>  - Indent switch statements
>>  - Use a scoped enum for type categories (as per the HotSpot style guide)
>
> 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"?

Right, it was required in a C-style unscoped-enum because of collisions with other enums in type.hpp. But I found in the HotSpot Style Guide that scoped-enums are actually encouraged, so I changed to that.

> src/hotspot/share/opto/idealGraphPrinter.cpp line 394:
>
>> 392:     }
>> 393:
>> 394:     switch (t->category()) {
>
> Might be more readable if switch cases are indented.

Done.

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

Done.

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

PR: https://git.openjdk.java.net/jdk/pull/2499
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8261336: IGV: enhance default filters [v2]

Roberto Castañeda Lozano-2
In reply to this post by Roberto Castañeda Lozano-2
On Fri, 12 Feb 2021 15:34:51 GMT, Roberto Castañeda Lozano <[hidden email]> wrote:

> > 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.
>
> Thanks for reviewing Christian! I will have a look at your comments on Monday.

I have addressed them now, please re-review.

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

PR: https://git.openjdk.java.net/jdk/pull/2499
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8261336: IGV: enhance default filters [v2]

Roberto Castañeda Lozano-2
In reply to this post by Christian Hagedorn-3
On Fri, 12 Feb 2021 12:13:13 GMT, Christian Hagedorn <[hidden email]> wrote:

>> Roberto Castañeda Lozano has updated the pull request incrementally with five additional commits since the last revision:
>>
>>  - Rewrite 'Show control flow only' filter using categories
>>  - Add leading underscore field
>>  - Move assertion to a default switch case
>>  - Indent switch statements
>>  - Use a scoped enum for type categories (as per the HotSpot style guide)
>
> 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);

Thanks for the suggestion, I adopted it with some variations to avoid having to explicitly list pinned nodes like "Phi".

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

PR: https://git.openjdk.java.net/jdk/pull/2499
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8261336: IGV: enhance default filters [v2]

Christian Hagedorn-3
In reply to this post by Roberto Castañeda Lozano-2
On Mon, 15 Feb 2021 11:15:11 GMT, Roberto Castañeda Lozano <[hidden email]> 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).
>
> Roberto Castañeda Lozano has updated the pull request incrementally with five additional commits since the last revision:
>
>  - Rewrite 'Show control flow only' filter using categories
>  - Add leading underscore field
>  - Move assertion to a default switch case
>  - Indent switch statements
>  - Use a scoped enum for type categories (as per the HotSpot style guide)

Thanks for addressing my suggestions. I tested it again - looks good!

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

Marked as reviewed by chagedorn (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/2499
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8261336: IGV: enhance default filters [v2]

Christian Hagedorn-3
In reply to this post by Roberto Castañeda Lozano-2
On Mon, 15 Feb 2021 11:19:04 GMT, Roberto Castañeda Lozano <[hidden email]> wrote:

>> 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);
>
> Thanks for the suggestion, I adopted it with some variations to avoid having to explicitly list pinned nodes like "Phi".

That's even better! Thanks for adopting that suggestion.

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

PR: https://git.openjdk.java.net/jdk/pull/2499
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8261336: IGV: enhance default filters [v2]

Tobias Hartmann-3
In reply to this post by Roberto Castañeda Lozano-2
On Mon, 15 Feb 2021 11:15:11 GMT, Roberto Castañeda Lozano <[hidden email]> 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).
>
> Roberto Castañeda Lozano has updated the pull request incrementally with five additional commits since the last revision:
>
>  - Rewrite 'Show control flow only' filter using categories
>  - Add leading underscore field
>  - Move assertion to a default switch case
>  - Indent switch statements
>  - Use a scoped enum for type categories (as per the HotSpot style guide)

This is awesome. Thanks a lot for taking the time to improve the IGV. Looks good to me!

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

Marked as reviewed by thartmann (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/2499
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8261336: IGV: enhance default filters [v2]

Roberto Castañeda Lozano-2
On Tue, 16 Feb 2021 10:33:10 GMT, Tobias Hartmann <[hidden email]> wrote:

>> Roberto Castañeda Lozano has updated the pull request incrementally with five additional commits since the last revision:
>>
>>  - Rewrite 'Show control flow only' filter using categories
>>  - Add leading underscore field
>>  - Move assertion to a default switch case
>>  - Indent switch statements
>>  - Use a scoped enum for type categories (as per the HotSpot style guide)
>
> This is awesome. Thanks a lot for taking the time to improve the IGV. Looks good to me!

Thanks Christian and Tobias for reviewing!

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

PR: https://git.openjdk.java.net/jdk/pull/2499
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8261336: IGV: enhance default filters [v2]

Roberto Castañeda Lozano-2
On Tue, 16 Feb 2021 12:42:45 GMT, Roberto Castañeda Lozano <[hidden email]> wrote:

>> This is awesome. Thanks a lot for taking the time to improve the IGV. Looks good to me!
>
> Thanks Christian and Tobias for reviewing!

Add filters to color and hide parts of the graph based on node categories or
estimated execution frequency, and simplify remaining filters.

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

PR: https://git.openjdk.java.net/jdk/pull/2499
Reply | Threaded
Open this post in threaded view
|

Integrated: 8261336: IGV: enhance default filters

Roberto Castañeda Lozano-2
In reply to this post by Roberto Castañeda Lozano-2
On Wed, 10 Feb 2021 10:00:00 GMT, Roberto Castañeda Lozano <[hidden email]> 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).

This pull request has now been integrated.

Changeset: 16bd7d38
Author:    Roberto Castañeda Lozano <[hidden email]>
URL:       https://git.openjdk.java.net/jdk/commit/16bd7d38
Stats:     375 lines in 25 files changed: 292 ins; 44 del; 39 mod

8261336: IGV: enhance default filters

Add filters to color and hide parts of the graph based on node categories or
estimated execution frequency, and simplify remaining filters.

Co-authored-by: Christian Hagedorn <[hidden email]>
Reviewed-by: vlivanov, chagedorn, thartmann

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

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