RFR: 8302738: IGV: refine 'Simplify graph' filter [v2]

[Tobias Holenstein]

On Mon, 27 Mar 2023 10:47:44 GMT, Roberto Castañeda Lozano <rcastanedalo at openjdk.org> wrote:

>> src/utils/IdealGraphVisualizer/Filter/src/main/java/com/sun/hotspot/igv/filter/CombineFilter.java line 71:
>> 
>>> 69:                             }
>>> 70:                         }
>>> 71: 
>> 
>> I think `assert slot != null;` should be moved up here
>
> Makes sense, but I did not change it because the surrounding code is essentially dead (no current filter has a "reversed" `CombineRule`) and I would not be able to test it. Since this code has not been executed for years, it is likely to be broken anyway.

okey

>> src/utils/IdealGraphVisualizer/Filter/src/main/java/com/sun/hotspot/igv/filter/FilterChain.java line 1:
>> 
>>> 1: /*
>> 
>> I think `applyInOrder` can be simplified as this :
>> 
>> public void applyInOrder(Diagram d, FilterChain sequence) {
>>         for (Filter f : sequence.getFilters()) {
>>             if (filters.contains(f)) {
>>                 f.apply(d);
>>             }
>>         }
>>     }
>> 
>> 
>> Reason: `FilterChain ordering` is the same as `this` in `FilterChain`. Usually `filters` are already in the order that we want them to apply. Only exception is when the user manually reoders the filters. `FilterChain sequence` contains all the filters in the order that they appear in the list. `filters` are the filters that are selected by the user and should alway be a subset of `sequence`. Therefore we can just iterate through `sequence` to get the correct order and apply each filter that is selected (contained in `filters`)
>
> Thanks for the suggestion! I tested your assumption ("`FilterChain ordering` is the same as `this` in `FilterChain`") but it does not hold in this PR. Note that the filter list is never ordered outside of `applyInOrder`. In any case, as I mentioned in https://github.com/openjdk/jdk/pull/12714#discussion_r1148879518, I propose to go with the fix in JDK-8302644 and discard the filter ordering changes from this PR.

sounds good

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

PR Review Comment: https://git.openjdk.org/jdk/pull/12955#discussion_r1150326349
PR Review Comment: https://git.openjdk.org/jdk/pull/12955#discussion_r1150327420