RFR: 8302738: IGV: refine 'Simplify graph' filter [v2]
Roberto Castañeda Lozano
rcastanedalo at openjdk.org
Mon Mar 27 11:09:34 UTC 2023
On Mon, 20 Mar 2023 10:13:15 GMT, Tobias Holenstein <tholenstein at openjdk.org> wrote:
>> Roberto Castañeda Lozano has updated the pull request incrementally with five additional commits since the last revision:
>>
>> - Increase the bold text line factor slightly
>> - Add extra horizontal margin for long labels and let them overflow within the node
>> - Select slots as well
>> - Remove code that is commented out
>> - Assert inputLabel is non-null
>
> 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.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/12955#discussion_r1149152222
More information about the hotspot-compiler-dev
mailing list