JDK-8293480: IGV: Update Bytecode and ControlFlow Component immediately when opening a new graph #10196

tobiasholenstein · 2022-09-07T09:36:02Z

The BytecodeViewTopComponent and ControlFlowTopComponent represent information depending on which graph is opened in EditorTopComponent. Previously BytecodeViewTopComponent and ControlFlowTopComponent did not update their contents immediately when a new graph from another group was opened in EditorTopComponent. They were also not updated when switching between two tabs of an open graph.
OutlineTopComponent had the same problem to update the selected graphs according to the EditorTopComponent

Analysis
BytecodeViewTopComponent, ControlFlowTopComponent and OutlineTopComponent each represent information that depends on the InputGraph of the currently or last active EditorTopComponent. This information is made available globally by adding a new InputGraphProvider to the Lookup of EditorTopComponent each time the InputGraph changes. BytecodeViewTopComponent, ControlFlowTopComponent and OutlineTopComponent implement a LookupListener that calls resultChanged(LookupEvent lookupEvent) whenever a InputGraphProvider changes in in the lookup of Utilities.actionsGlobalContext(). When such a change happens the last active InputGraphProvider is retrieved from the LookupHistory by the listening components.

Problem
First, we missed to fire() a diagramChangedEvent in the constructor of EditorTopComponent which trigger to add the InputGraphProvider to the Lookup

Second, Utilities.actionsGlobalContext() returns a Lookup of the active (focused) TopComponent's Lookup. Unfortunately, when the last EditorTopComponent the LookupListener does not get called and there is no way to call is manually.

New Approach
We extends the LookupHistory class such that we can add a ChangedListener that gets called whenever the last active InputGraphProvider is cached. So instead of listening to changes in Utilities.actionsGlobalContext() and then consulting the LookupHistory, now BytecodeViewTopComponent, ControlFlowTopComponent and OutlineTopComponent listen to changes in the LookupHistory directly. This way we can now call terminate in the LookupHistory whenever we close a EditorTopComponent, which directly notifies the listeners.

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8293480: IGV: Update Bytecode and ControlFlow Component immediately when opening a new graph

Reviewers

Roberto Castañeda Lozano (@robcasloz - Reviewer) ⚠️ Review applies to 31b9d673
Christian Hagedorn (@chhagedorn - Reviewer) ⚠️ Review applies to 31b9d673

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk pull/10196/head:pull/10196
$ git checkout pull/10196

Update a local copy of the PR:
$ git checkout pull/10196
$ git pull https://git.openjdk.org/jdk pull/10196/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 10196

View PR using the GUI difftool:
$ git pr show -t 10196

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/10196.diff

… new graph

bridgekeeper · 2022-09-07T09:36:57Z

👋 Welcome back tholenstein! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2022-09-07T09:39:15Z

@tobiasholenstein The following label will be automatically applied to this pull request:

hotspot-compiler

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

mlbridge · 2022-09-07T10:58:22Z

Webrevs

robcasloz

Thanks for this UI improvement, Tobias, looks good to me! There is one more case where the Bytecode and Control Flow windows get out of sync: after removing all graphs and groups in the Outline, they still show the content of the graph that was last active:

This problem existed before the changeset, so it might be addressed here or in a separate issue, whatever you think makes more sense.

...sualizer/Bytecodes/src/main/java/com/sun/hotspot/igv/bytecodes/BytecodeViewTopComponent.java

...lizer/ControlFlow/src/main/java/com/sun/hotspot/igv/controlflow/ControlFlowTopComponent.java

...isualizer/Coordinator/src/main/java/com/sun/hotspot/igv/coordinator/OutlineTopComponent.java

openjdk · 2022-09-08T11:55:54Z

@tobiasholenstein This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8293480: IGV: Update Bytecode and ControlFlow Component immediately when opening a new graph

Reviewed-by: rcastanedalo, chagedorn

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 277 new commits pushed to the master branch:

8ecdaa6: 8294000: Filler array klass should be in jdk/vm/internal, not in java/vm/internal
379f309: 8287217: C2: PhaseCCP: remove not visited nodes, prevent type inconsistency
12e3510: 8293798: Fix test bugs due to incompatibility with -XX:+AlwaysIncrementalInline
cb72f80: 8293978: Duplicate simple loop back-edge will crash the vm
cddd6de: 8279941: sun/security/pkcs11/Signature/TestDSAKeyLength.java fails when NSS version detection fails
21008ca: 8285383: vmTestbase/nsk/jvmti/scenarios/hotswap/HS204/hs204t001/hs204t001.java failed with "exit code: 96"
3b438a6: 8294067: [macOS] javax/swing/JComboBox/6559152/bug6559152.java Cannot select an item from popup with the ENTER key.
caae53f: 8289508: Improve test coverage for XPath Axes: ancestor, ancestor-or-self, preceding, and preceding-sibling
cb5771d: 8294006: Avoid hardcoding object file suffixes in make
5002eaa: 8293828: JFR: jfr/event/oldobject/TestClassLoaderLeak.java still fails when GC cycles are not happening
... and 267 more: https://git.openjdk.org/jdk/compare/512fee1d1eee4aa4bb7362cc9cb48d63e129a525...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

tobiasholenstein · 2022-09-09T13:06:16Z

Thanks for this UI improvement, Tobias, looks good to me! There is one more case where the Bytecode and Control Flow windows get out of sync: after removing all graphs and groups in the Outline, they still show the content of the graph that was last active:

This problem existed before the changeset, so it might be addressed here or in a separate issue, whatever you think makes more sense.

Thanks for spotting that @robcasloz! I fixed it now. As a by-product of my fix, the OutlineTopComponent now also removes the graph selection when the corresponding EditorTopComponent was closed

robcasloz

Thanks for addressing the additional case, Tobias. I found an issue in the new revision, though: the graph selected in the Outline is not updated when clicking on the "Show previous / next graph of the current group" buttons. Furthermore, after clicking a few times forward and backwards (~20 times or more), IGV becomes very unresponsive (tens of seconds to update the graph view).

This reverts commit e908804.

This reverts commit 49dbaa3.

tobiasholenstein · 2022-09-16T15:41:18Z

fix

Thanks for this UI improvement, Tobias, looks good to me! There is one more case where the Bytecode and Control Flow windows get out of sync: after removing all graphs and groups in the Outline, they still show the content of the graph that was last active:

This problem existed before the changeset, so it might be addressed here or in a separate issue, whatever you think makes more sense.

You are right, @robcasloz, my last fix didn't solve the problem. There was a fundamental problem with using Utilities.actionsGlobalContext() - I pushed a new version and updated the PR.

Thanks!

robcasloz

I tried out the latest changes and it works as expected, thanks! However, I found one more case where the Control Flow window get out of sync with the active graph: if you open a graph and then go to the Outline and click on "Remove selected graphs and groups" (where only the opened graph is selected), the active graph changes to another graph in the same group but the Control Flow window still shows the CFG of the removed graph. Even though this problem is not introduced by this changeset, I suggest addressing it here.

Also, please make sure that the NetBeans-generated code moved by this changeset is still editable in its new context.

tobiasholenstein · 2022-09-20T13:10:33Z

I tried out the latest changes and it works as expected, thanks! However, I found one more case where the Control Flow window get out of sync with the active graph: if you open a graph and then go to the Outline and click on "Remove selected graphs and groups" (where only the opened graph is selected), the active graph changes to another graph in the same group but the Control Flow window still shows the CFG of the removed graph. Even though this problem is not introduced by this changeset, I suggest addressing it here.

Also, please make sure that the NetBeans-generated code moved by this changeset is still editable in its new context.

Thanks for trying it out! You are right. It also happens when deleting a graph with a smaller index number in the same group. The reason is that the RangeSliderModel saves the index of the current open graph. If the index number changes (what happens when you delete a graph with index <= current index), during repainting a different graph is painted.

If you don't mind I prefer filing a Bug for this and solve it separately

robcasloz · 2022-09-20T13:20:38Z

If you don't mind I prefer filing a Bug for this and solve it separately

Sure, makes sense given that the bug seems to have a different root cause.

tobiasholenstein · 2022-09-20T13:46:15Z

If you don't mind I prefer filing a Bug for this and solve it separately

Sure, makes sense given that the bug seems to have a different root cause.

I filed the bug: https://bugs.openjdk.org/browse/JDK-8294066

chhagedorn

Otherwise, looks good!

...isualizer/Coordinator/src/main/java/com/sun/hotspot/igv/coordinator/OutlineTopComponent.java

src/utils/IdealGraphVisualizer/Util/src/main/java/com/sun/hotspot/igv/util/LookupHistory.java

...ealGraphVisualizer/View/src/main/java/com/sun/hotspot/igv/view/EditorInputGraphProvider.java

...ils/IdealGraphVisualizer/View/src/main/java/com/sun/hotspot/igv/view/EditorTopComponent.java

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

Co-authored-by: Christian Hagedorn <christian.hagedorn@oracle.com>

…pot/igv/util/LookupHistory.java remove this Co-authored-by: Christian Hagedorn <christian.hagedorn@oracle.com>

Co-authored-by: Christian Hagedorn <christian.hagedorn@oracle.com>

tobiasholenstein · 2022-09-21T08:04:45Z

@robcasloz I checked that NetBeans-generated is still editable.

@chhagedorn I have applied the code style changes you suggested.

tobiasholenstein · 2022-09-21T08:07:04Z

@robcasloz and @chhagedorn thanks for the reviews!

/integrate

openjdk · 2022-09-21T08:08:20Z

Going to push as commit 4e7cb15.
Since your change was applied there have been 277 commits pushed to the master branch:

8ecdaa6: 8294000: Filler array klass should be in jdk/vm/internal, not in java/vm/internal
379f309: 8287217: C2: PhaseCCP: remove not visited nodes, prevent type inconsistency
12e3510: 8293798: Fix test bugs due to incompatibility with -XX:+AlwaysIncrementalInline
cb72f80: 8293978: Duplicate simple loop back-edge will crash the vm
cddd6de: 8279941: sun/security/pkcs11/Signature/TestDSAKeyLength.java fails when NSS version detection fails
21008ca: 8285383: vmTestbase/nsk/jvmti/scenarios/hotswap/HS204/hs204t001/hs204t001.java failed with "exit code: 96"
3b438a6: 8294067: [macOS] javax/swing/JComboBox/6559152/bug6559152.java Cannot select an item from popup with the ENTER key.
caae53f: 8289508: Improve test coverage for XPath Axes: ancestor, ancestor-or-self, preceding, and preceding-sibling
cb5771d: 8294006: Avoid hardcoding object file suffixes in make
5002eaa: 8293828: JFR: jfr/event/oldobject/TestClassLoaderLeak.java still fails when GC cycles are not happening
... and 267 more: https://git.openjdk.org/jdk/compare/512fee1d1eee4aa4bb7362cc9cb48d63e129a525...master

Your commit was automatically rebased without conflicts.

openjdk · 2022-09-21T08:08:39Z

@tobiasholenstein Pushed as commit 4e7cb15.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

Updated Bytecode and ControlFlow Component immediately when opening a…

3d06f78

… new graph

tobiasholenstein changed the title ~~JDK-8293480: IGV: Updated Bytecode and ControlFlow Component immediately when opening a new graph~~ JDK-8293480: IGV: Update Bytecode and ControlFlow Component immediately when opening a new graph Sep 7, 2022

openjdk bot added the hotspot-compiler hotspot-compiler-dev@openjdk.org label Sep 7, 2022

tobiasholenstein marked this pull request as ready for review September 7, 2022 10:54

openjdk bot added the rfr Pull request is ready for review label Sep 7, 2022

robcasloz approved these changes Sep 8, 2022

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Sep 8, 2022

use multi-line lambdas

ac0d42a

robcasloz approved these changes Sep 8, 2022

View reviewed changes

Tobias Holenstein added 2 commits September 8, 2022 18:05

update TopComponents on closing

49dbaa3

Update Bytecode and ControlFlow when a group is removed

e908804

robcasloz suggested changes Sep 14, 2022

View reviewed changes

openjdk bot removed the ready Pull request is ready to be integrated label Sep 14, 2022

Tobias Holenstein added 6 commits September 15, 2022 17:01

Revert "Update Bytecode and ControlFlow when a group is removed"

cf4e789

This reverts commit e908804.

Revert "update TopComponents on closing"

2a64316

This reverts commit 49dbaa3.

update OutlineTopComponent graph closed

9735afd

add ChangedListener to LookupHistory

fbe5566

remove DiagramProvider

020e097

Fix for: parent is null for DiffGraph

31b9d67

robcasloz approved these changes Sep 20, 2022

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Sep 20, 2022

chhagedorn approved these changes Sep 20, 2022

View reviewed changes

tobiasholenstein and others added 5 commits September 21, 2022 09:29

remove whitespace

9ad5cb2

Co-authored-by: Christian Hagedorn <christian.hagedorn@oracle.com>

Update src/utils/IdealGraphVisualizer/Util/src/main/java/com/sun/hots…

544bf1d

…pot/igv/util/LookupHistory.java remove this Co-authored-by: Christian Hagedorn <christian.hagedorn@oracle.com>

code style

7ff0d3f

Co-authored-by: Christian Hagedorn <christian.hagedorn@oracle.com>

code style: remove "this."

53c553e

Co-authored-by: Christian Hagedorn <christian.hagedorn@oracle.com>

code style : remove "this."

6880342

Co-authored-by: Christian Hagedorn <christian.hagedorn@oracle.com>

openjdk bot added the integrated Pull request has been integrated label Sep 21, 2022

openjdk bot closed this Sep 21, 2022

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Sep 21, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

JDK-8293480: IGV: Update Bytecode and ControlFlow Component immediately when opening a new graph #10196

JDK-8293480: IGV: Update Bytecode and ControlFlow Component immediately when opening a new graph #10196

tobiasholenstein commented Sep 7, 2022 •

edited by openjdk bot

Loading

bridgekeeper bot commented Sep 7, 2022

openjdk bot commented Sep 7, 2022

mlbridge bot commented Sep 7, 2022 •

edited

Loading

robcasloz left a comment

openjdk bot commented Sep 8, 2022 •

edited

Loading

tobiasholenstein commented Sep 9, 2022

robcasloz left a comment

tobiasholenstein commented Sep 16, 2022

robcasloz left a comment

tobiasholenstein commented Sep 20, 2022

robcasloz commented Sep 20, 2022

tobiasholenstein commented Sep 20, 2022

chhagedorn left a comment

tobiasholenstein commented Sep 21, 2022

tobiasholenstein commented Sep 21, 2022

openjdk bot commented Sep 21, 2022

openjdk bot commented Sep 21, 2022

JDK-8293480: IGV: Update Bytecode and ControlFlow Component immediately when opening a new graph #10196

JDK-8293480: IGV: Update Bytecode and ControlFlow Component immediately when opening a new graph #10196

Conversation

tobiasholenstein commented Sep 7, 2022 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Sep 7, 2022

openjdk bot commented Sep 7, 2022

mlbridge bot commented Sep 7, 2022 • edited Loading

Webrevs

robcasloz left a comment

Choose a reason for hiding this comment

openjdk bot commented Sep 8, 2022 • edited Loading

tobiasholenstein commented Sep 9, 2022

robcasloz left a comment

Choose a reason for hiding this comment

tobiasholenstein commented Sep 16, 2022

robcasloz left a comment

Choose a reason for hiding this comment

tobiasholenstein commented Sep 20, 2022

robcasloz commented Sep 20, 2022

tobiasholenstein commented Sep 20, 2022

chhagedorn left a comment

Choose a reason for hiding this comment

tobiasholenstein commented Sep 21, 2022

tobiasholenstein commented Sep 21, 2022

openjdk bot commented Sep 21, 2022

openjdk bot commented Sep 21, 2022

tobiasholenstein commented Sep 7, 2022 •

edited by openjdk bot

Loading

mlbridge bot commented Sep 7, 2022 •

edited

Loading

openjdk bot commented Sep 8, 2022 •

edited

Loading