RFR: 8291801: IGV: Broken button "Hide graphs which are the same as the previous graph" [v2]

[Tobias Holenstein]

On Wed, 28 Sep 2022 06:51:37 GMT, Koichi Sakata <ksakata at openjdk.org> wrote:

>> This pull request makes the "Hide graphs..." button working.
>> 
>> The parser for bgv files, `BinaryParaser` class, enables the "hide graphs" function. But the xml parser, `Parser` class, doesn't have any code about the function. So I wrote code by referring to BinaryParser class.
>> 
>> # Tests
>> 
>> I tested manually. Screenshots are as follows.
>> 
>> <img width="1175" alt="スクリーンショット 2022-09-27 16 52 16" src="https://user-images.githubusercontent.com/60008/192467385-328b8298-9055-484d-813d-1c50200e2239.png">
>> 
>> In this case, there are 13 graphs and 10 graphs from "After Parsing" to "Before matching" are the same. So only 4 graphs are shown after we push the button. 
>> 
>> <img width="1175" alt="スクリーンショット 2022-09-27 16 52 23" src="https://user-images.githubusercontent.com/60008/192467331-fbeb4911-460d-476c-9445-098805c39d27.png">
>> 
>> Pushing it again or opening the graph that is hidden in the view restores the view to its original state. I attached [the graph file that I used in the test](https://github.com/openjdk/jdk/files/9653179/hello.zip).
>> 
>> Furthermore, I added a test method for the class I changed. Running `mvn test` command was successful.
>> 
>> # Concerns
>> 
>> I'm concerned about the equivalence of 2 graphs. I regard them as the same graphs when all fields in `InputGraph` class are equal. But in the test class, 2 graphs are equal when nodes, edges, blocks and blocks of each `InputNode` are equal. Here is an extract of the test code.
>> 
>> 
>> // src/utils/IdealGraphVisualizer/Data/src/test/java/com/sun/hotspot/igv/data/Util.java
>> 
>>     public static void assertGraphEquals(InputGraph a, InputGraph b) {
>> 
>>         if(!a.getNodesAsSet().equals(b.getNodesAsSet())) {
>>             fail();
>>         }
>> 
>>         if (!a.getEdges().equals(b.getEdges())) {
>>             fail();
>>         }
>> 
>>         if (a.getBlocks().equals(b.getBlocks())) {
>>             fail();
>>         }
>> 
>>         for (InputNode n : a.getNodes()) {
>>             assertEquals(a.getBlock(n), b.getBlock(n));
>>         }
>>     }
>> 
>> 
>> But opening a graph is very slow when I compare blocks of each InputNode. So I didn't add that comparison in `isSameContent` method.
>
> Koichi Sakata has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains seven additional commits since the last revision:
> 
>  - Set value to hideDuplicates field
>  - Merge remote-tracking branch 'upstream/master' into JDK-8291801
>  - Remove extra whitespaces
>  - Improve the test
>  - Add the test method
>  - Remove extra code
>  - Fix the broken button "Hide graphs which are the same as the previous graph" in IGV

Hi Koichi,

I have also noticed that the following `Objects.equals` behaved differently on different platforms.

&& Objects.equals(this.parent, i.parent)
&& Objects.equals(this.parentGroup, i.parentGroup)
&& Objects.equals(this.blocks, i.blocks)
&& Objects.equals(this.blockEdges, i.blockEdges)
&& Objects.equals(this.nodeToBlock, i.nodeToBlock)
&& Objects.equals(this.isDiffGraph, i.isDiffGraph);


I think it's actually enough to check that the nodes and edges are the same:

return Objects.equals(this.nodes, i.nodes)
            && Objects.equals(this.edges, i.edges);


Since the `InputNode` in `nodes` extends `Properties.Entity` we ideally also check that two `InputNode` have the same properties. One way could be to change the `equals(Object o)` function in  InputNode.java to do that:

return n.id == id && n.getProperties().equals(getProperties());


But I am not sure if the `InputNode` `equals` function is used anywhere else in the code. So I would prefer to check for equality of the `nodes` explicitly in `isSameContent` without using `Objects.equals`.

Just for inspiration: In `Difference.java` we do something similar. But I think since we are only checking for equality and not calculating the difference of two graphs, it could be much less complex.  

BTW: `duplicates_example.xml` contains two example graphs, that I expected to be duplicates:
[duplicates_example.zip](https://github.com/openjdk/jdk/files/9676215/duplicates_example.zip)

One last thing: For large graphs, the `isSameContent` could be costly. Users will not always use `Hide graphs which are the same as the previous graph` and perhaps there is a way to offload the calculation from the parser to only calculate `_isDuplicate` lazily when used in `filterGraphs()` and `setHideDuplicates(boolean hideDuplicates)` in `DiagramViewModel`.

Thank you, and feel free to ask if anything I have written is unclear.

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

PR: https://git.openjdk.org/jdk/pull/10440