8323965: modify fix for 8317771 to remove reflection instantiation of the inner class #18867

savoptik · 2024-04-19T15:16:41Z

I replaced reflection with using an accessor
@azuev-java please review

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

Warning

⚠️ Found leading lowercase letter in issue title for 8323965: modify fix for 8317771 to remove reflection instantiation of the inner class

Issues

JDK-8323965: modify fix for 8317771 to remove reflection instantiation of the inner class (Bug - P4)
JDK-8329667: [macos] Issue with JTree related fix for JDK-8317771 (Bug - P3) ⚠️ Issue is already resolved. Consider making this a "backport pull request" by setting the PR title to Backport <hash> with the hash of the original commit. See Backports.

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 18867

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

Using diff file

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

Webrev

Link to Webrev Comment

…the inner class

bridgekeeper · 2024-04-19T15:17:38Z

👋 Welcome back asemenov! 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 · 2024-04-19T15:18:32Z

❗ This change is not yet ready to be integrated.
See the Progress checklist in the description for automated requirements.

openjdk · 2024-04-19T15:19:08Z

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

client

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.

savoptik · 2024-04-19T15:20:54Z

/issue add 8322239

openjdk · 2024-04-19T15:21:20Z

@savoptik
Adding additional issue to issue list: 8322239: [macos] a11y : java.lang.NullPointerException is thrown when focus is moved on the JTabbedPane.

mlbridge · 2024-04-19T15:27:19Z

Webrevs

00: Full (7282771f)

savoptik · 2024-04-19T15:27:36Z

/issue remove8322239

openjdk · 2024-04-19T15:28:59Z

@savoptik Command syntax:

/issue [add|remove] <id>[,<id>,...]
/issue [add] <id>: <description>
`/issue create [PX] /[subcomponent]

Some examples:

/issue add JDK-1234567,4567890
/issue remove JDK-4567890
/issue 1234567: Use this exact title
`/issue create hotspot/jfr
`/issue create P4 core-libs/java.nio

If issues are specified only by their ID, the title will be automatically retrieved from JBS. The project prefix (JDK- in the above examples) is optional. Separate multiple issue IDs using either spaces or commas.

savoptik · 2024-04-19T15:30:08Z

/issue remove 8322239

openjdk · 2024-04-19T15:31:36Z

@savoptik
Removing additional issue from issue list: 8322239.

savoptik · 2024-04-19T15:33:33Z

/issue add 8329667

openjdk · 2024-04-19T15:33:38Z

@savoptik
Adding additional issue to issue list: 8329667: [macos] Issue with JTree related fix for JDK-8317771.

liach · 2024-04-19T20:24:38Z

src/java.desktop/share/classes/sun/swing/SwingAccessor.java

+    public static AccessibleJTreeNodeCreateAccessor getAccessibleJTreeNodeCreateAccessor() {
+        var access = accessibleJTreeNodeCreateAccessor;
+        if (access == null) {
+            ensureClassInitialized(JTree.class);


This probably doesn't work as JTree class initialization does not seem to trigger AccessibleJTree class initialization. The other one for AccessibleJTreeNode is dubious too, maybe it just happens that all accesses are correctly made only after the related classes are initialized.

This line calls the ensureClassInitialized() method, which performs a full initialization of the class... It calls: MethodHandles.lookup().ensureInitialized(), which, as stated in the documentation , performs a full initialization.

I know, but initializing a class does not necessarily initialize its nested/inner classes

azuev-java · 2024-04-23T02:25:57Z

The problem with this fix is that on the test example attached to the bug any attempt of navigation trough the items of JTree whole voice over is enabled causes java to stop responding. I see in the logs that it does call this exact place thousands of time constantly. So it seems like it makes the problem with java stalling on large size trees to re-appear.

savoptik · 2024-04-23T07:10:50Z

@azuev-java
I'm restoring the context: There was a cycle that recursively collected children, and on new MacOS it worked for a very long time.. JDK-8317771 .
I added a solution for JTree, which works much faster, but there was a reflection, you asked to remove it. Unlike the old algorithm, it now works in seconds, not minutes...
The essence of JDK-8329667 is that the custom tree did not work due to reflection.
I removed the reflection, I did not add any additional acceleration.
As for speeding up the tree, I suggest adding caching, similar to how it is implemented in tables.

azuev-java · 2024-04-24T21:59:47Z

src/java.desktop/share/classes/javax/swing/JTree.java

@@ -4272,6 +4272,10 @@ protected class AccessibleJTree extends AccessibleJComponent
        TreePath   leadSelectionPath;
        Accessible leadSelectionAccessible;

+        static {
+            SwingAccessor.setAccessibleJTreeNodeCreateAccessor(new AccessibleTreNodeACreateccessor());


I think there is a typo here. AccessibleTreNodeACreateccessor does not sound right. Should be AccessibleTreeNodeCreateAccessor.

bridgekeeper · 2024-05-22T23:51:11Z

@savoptik This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

openjdk · 2024-05-25T00:39:02Z

@savoptik this pull request can not be integrated into master due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:

git checkout asemenov/JDK-8323965
git fetch https://git.openjdk.org/jdk.git master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push

bridgekeeper · 2024-06-22T00:53:30Z

@savoptik This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

bridgekeeper · 2024-07-20T01:23:43Z

@savoptik This pull request has been inactive for more than 8 weeks and will now be automatically closed. If you would like to continue working on this pull request in the future, feel free to reopen it! This can be done using the /open pull request command.

8323965 modify fix for 8317771 to remove reflection instantiation of …

Loading
Loading status checks…

7282771

…the inner class

openjdk bot added the rfr label Apr 19, 2024

openjdk bot added the client label Apr 19, 2024

savoptik changed the title ~~8323965 modify fix for 8317771 to remove reflection instantiation of …the inner class~~ 8323965 modify fix for 8317771 to remove reflection instantiation of the inner class Apr 19, 2024

openjdk bot changed the title ~~8323965 modify fix for 8317771 to remove reflection instantiation of the inner class~~ 8323965: modify fix for 8317771 to remove reflection instantiation of the inner class Apr 19, 2024

liach reviewed Apr 19, 2024

View reviewed changes

azuev-java reviewed Apr 24, 2024

View reviewed changes

openjdk bot added the merge-conflict label May 25, 2024

openjdk bot removed the rfr label Jun 6, 2024

bridgekeeper bot closed this Jul 20, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8323965: modify fix for 8317771 to remove reflection instantiation of the inner class #18867

8323965: modify fix for 8317771 to remove reflection instantiation of the inner class #18867

savoptik commented Apr 19, 2024 •

edited by openjdk bot

Loading

bridgekeeper bot commented Apr 19, 2024

openjdk bot commented Apr 19, 2024

openjdk bot commented Apr 19, 2024

savoptik commented Apr 19, 2024

openjdk bot commented Apr 19, 2024

mlbridge bot commented Apr 19, 2024

savoptik commented Apr 19, 2024

openjdk bot commented Apr 19, 2024

savoptik commented Apr 19, 2024

openjdk bot commented Apr 19, 2024

savoptik commented Apr 19, 2024

openjdk bot commented Apr 19, 2024

liach Apr 19, 2024

savoptik Apr 22, 2024

liach Apr 22, 2024

azuev-java commented Apr 23, 2024

savoptik commented Apr 23, 2024

azuev-java Apr 24, 2024

bridgekeeper bot commented May 22, 2024

openjdk bot commented May 25, 2024

bridgekeeper bot commented Jun 22, 2024

bridgekeeper bot commented Jul 20, 2024

8323965: modify fix for 8317771 to remove reflection instantiation of the inner class #18867

8323965: modify fix for 8317771 to remove reflection instantiation of the inner class #18867

Conversation

savoptik commented Apr 19, 2024 • edited by openjdk bot Loading

Progress

Warning

Issues

Reviewing

Webrev

bridgekeeper bot commented Apr 19, 2024

openjdk bot commented Apr 19, 2024

openjdk bot commented Apr 19, 2024

savoptik commented Apr 19, 2024

openjdk bot commented Apr 19, 2024

mlbridge bot commented Apr 19, 2024

Webrevs

savoptik commented Apr 19, 2024

openjdk bot commented Apr 19, 2024

savoptik commented Apr 19, 2024

openjdk bot commented Apr 19, 2024

savoptik commented Apr 19, 2024

openjdk bot commented Apr 19, 2024

liach Apr 19, 2024

Choose a reason for hiding this comment

savoptik Apr 22, 2024

Choose a reason for hiding this comment

liach Apr 22, 2024

Choose a reason for hiding this comment

azuev-java commented Apr 23, 2024

savoptik commented Apr 23, 2024

azuev-java Apr 24, 2024

Choose a reason for hiding this comment

bridgekeeper bot commented May 22, 2024

openjdk bot commented May 25, 2024

bridgekeeper bot commented Jun 22, 2024

bridgekeeper bot commented Jul 20, 2024

savoptik commented Apr 19, 2024 •

edited by openjdk bot

Loading