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

[Tobias Holenstein]

On Wed, 18 Jan 2023 03:00:45 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 incrementally with one additional commit since the last revision:
> 
>   Remove extra whitespaces

Great work! Thanks for following up on the suggestions. I tested your code and it seems to work. Two small issues I found:
-  `_isDuplicate` should be recomputed when adding or deleting graphs (see my other comment)
-  When opening a graph that is currently hidden `setHideDuplicates(false)` is called which is correct in my opinion. Unfortunately, the `HideDuplicatesAction` of the "Hide graphs which are the same as the previous graph" button keeps being selected. One way this could be fixed is adding a listener to `HideDuplicatesAction` 

https://github.com/openjdk/jdk/blob/5a77e6aaadbf7e0deb31b7adae23b398c6c73562/src/utils/IdealGraphVisualizer/View/src/main/java/com/sun/hotspot/igv/view/actions/HideDuplicatesAction.java#L39

public HideDuplicatesAction(ChangedEvent<DiagramViewModel> graphChangedEvent) {
    graphChangedEvent.addListener(model -> putValue(SELECTED_KEY, model.getHideDuplicates()));

and then pass `graphChangedEvent` to `HideDuplicatesAction` at initialization
https://github.com/openjdk/jdk/blob/5a77e6aaadbf7e0deb31b7adae23b398c6c73562/src/utils/IdealGraphVisualizer/View/src/main/java/com/sun/hotspot/igv/view/EditorTopComponent.java#L210

toolBar.add(new JToggleButton(new HideDuplicatesAction(diagramViewModel.getGraphChangedEvent())));

src/utils/IdealGraphVisualizer/View/src/main/java/com/sun/hotspot/igv/view/DiagramViewModel.java line 470:

> 468:         };
> 469:     }
> 470: 

When **deleting** a graph or **adding** a new graph to a group (e.g. when stepping through VM code in a c++ debugger) the `_isDuplicate` property should be recomputed for correctness. Luckily we have `groupContentChangedListener` that is called in exactly those two cases. I therefore suggest to add 

ChangedListener<Group> groupContentChangedListener = g -> {
    this.markedDuplicates = false;
    this.hideDuplicates = false;

to the following line 
https://github.com/openjdk/jdk/blob/5a77e6aaadbf7e0deb31b7adae23b398c6c73562/src/utils/IdealGraphVisualizer/View/src/main/java/com/sun/hotspot/igv/view/DiagramViewModel.java#L174

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

Changes requested by tholenstein (Committer).

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