Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8252935: Add treeShowing listener only when needed #185

Closed

Conversation

hjohn
Copy link
Collaborator

@hjohn hjohn commented Apr 17, 2020

This is a PoC for performance testing.

It contains commented out code in PopupWindow and ProgressIndicatorSkin and two tests are failing because of that.

This code avoids registering two listeners (on Scene and on Window) for each and every Node to support the aforementioned classes. The complete change should update these two classes to add their own listeners instead.


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8252935: Add treeShowing listener only when needed

Reviewers

Download

$ git fetch https://git.openjdk.java.net/jfx pull/185/head:pull/185
$ git checkout pull/185

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Apr 17, 2020

👋 Welcome back jhendrikx! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request.

@kevinrushforth
Copy link
Member

@kevinrushforth kevinrushforth commented Apr 17, 2020

These listeners exist for a good reason. Removing them will almost certainly cause regressions in behavior. See JDK-8090322 as well as the follow-on fixes (since that was a rather involved fix in the first place).

@hjohn
Copy link
Collaborator Author

@hjohn hjohn commented Apr 17, 2020

@kevinrushforth I don't propose to remove them, only to move them to where they are needed.

Currently they are part of Node, which means all nodes, whether they need to know the state of the Window or not are registering a listener. However, only PopupWindow and the ProgressIndicatorSkin are registering a listener on this property (other uses are limited to a simple "isTreeShowing" checks which can be determined directly).

It is very wasteful to have all nodes register this listener, as there are potentially tens of thousands of nodes. All of these nodes are listening on the exact same two properties (when there is only one Scene and Window), causing huge listener lists there. The listener is not registered in a lazy fashion (ie, only registered if something else is listening to the property downstream, like reactfx would do).

This also means that when a Scene change or Window showing change occurs, thousands of nodes will receive a change event, but only 2 types of nodes are actually interested. The other nodes are just doing a lot of house keeping to keep watching the correct property (as there is an indirection from Scene to Window).

Therefore my proposal is to have the two cases involved register their own listener on Scene and Window. There will not be any regressions. The two tests that currently fail in this PR are expected to fail as I commented out the listener code in the classes involved, but that can easily be fixed by adding the correct listeners there.

I'll update it so you can see all tests will pass.

@hjohn hjohn force-pushed the feature/remove-is-tree-showing-listener branch from 4fd989d to 8b62140 Compare Apr 17, 2020
@hjohn
Copy link
Collaborator Author

@hjohn hjohn commented Apr 17, 2020

@kevinrushforth I've updated this PR now with proper listeners added in again for PopupWindow and ProgressIndicatorSkin. In short, the functionality to support the treeShowingProperty has been extracted to a separate class TreeShowingExpression which is now used by the two classes mentioned.

All tests pass, including the memory leak tests that failed before.

The issue JDK-8090322 you mentioned actually cautioned for not adding such listeners for all nodes and seemed to propose the kind of solution I constructed here with a separate class for the treeShowingProperty:

This is too expensive to calculate for all nodes by default. So the simplest way to provide it would be a special binding implementation or a util class. Where you create a instance and pass in the node you are interested in. It can then register listeners all the way up the tree and listen to what it needs.

@hjohn
Copy link
Collaborator Author

@hjohn hjohn commented Apr 21, 2020

@kevinrushforth

I have another working alternative, but won't make a PR for it until it is more clear which direction the TableView / TreeTableView performance fixes are going to take.

The alternative would convert the treeShowing property in Node into a "lazy" property (something which JavaFX does not support directly at the moment). A lazy property only binds to its dependencies when it is observed itself (so in this specific case, only when PopupWindow or ProgressIndicatorSkin are making use of it).

This means the property stays a part of NodeHelper but will only register its listeners on Window and Scene when it is observed itself. Such lazy properties could be of great use in JavaFX in general, not just in this case.

@kevinrushforth kevinrushforth self-requested a review Jul 23, 2020
@hjohn hjohn changed the title WIP: Remove tree showing listeners on Window and Scene for all nodes Remove tree showing listeners on Window and Scene for all nodes Aug 26, 2020
@kevinrushforth
Copy link
Member

@kevinrushforth kevinrushforth commented Sep 8, 2020

@hjohn Per this message on the openjfx-dev mailing list, I have filed a new JBS issue for this PR to use. Please change the title to:

8252935: Add treeShowing listener only when needed

@kevinrushforth
Copy link
Member

@kevinrushforth kevinrushforth commented Sep 8, 2020

/reviewers 2

@openjdk
Copy link

@openjdk openjdk bot commented Sep 8, 2020

@kevinrushforth
The number of required reviews for this PR is now set to 2 (with at least 1 of role reviewers).

@hjohn hjohn changed the title Remove tree showing listeners on Window and Scene for all nodes 8252935: Add treeShowing listener only when needed Sep 9, 2020
@openjdk openjdk bot added the rfr label Sep 9, 2020
@mlbridge
Copy link

@mlbridge mlbridge bot commented Sep 9, 2020

Webrevs

@kevinrushforth kevinrushforth requested a review from arapte Sep 10, 2020
@hjohn
Copy link
Collaborator Author

@hjohn hjohn commented Jan 20, 2021

So, will this actually get reviewed?

@kevinrushforth
Copy link
Member

@kevinrushforth kevinrushforth commented Jan 22, 2021

Now that we are past RDP1 for JavaFX 16, this seems a good time to look at some of these performance issues. One of the challenges will be ensuring no regressions.

I did a preliminary review of this, along with some testing, and it looks less scary than I was afraid it would be. The overall approach seems sound: preserve the NodeHelper::isTreeVisible method, but don't provide it as an observable property, instead adding the listeners only to those that need them.

Can you do the following?

  1. Merge in the latest changes from the upstream master branch into your local feature branch? Among other things, this will enable running tests via GitHub Actions.
  2. Add a test program under tests/performance that can be used to verify the performance gain. Even better would be if you can create an automated test under tests/system. I was thinking something like what we did for PR #34 where there was a pathological performance bug fixed and the test checked that the operation in question didn't take more than some small number of seconds. That might be overkill for this fix, though.

Copy link
Member

@kevinrushforth kevinrushforth left a comment

I left a few inline comments below.

@hjohn hjohn force-pushed the feature/remove-is-tree-showing-listener branch from 8b62140 to 5ac5d9e Compare Jan 24, 2021
@hjohn
Copy link
Collaborator Author

@hjohn hjohn commented Jan 24, 2021

Sorry about the force push, merging over a year of changes from master did not seem right to me. It was only for getting up to date, I will do the other commits normally.

@hjohn
Copy link
Collaborator Author

@hjohn hjohn commented Jan 24, 2021

  1. Merge in the latest changes from the upstream master branch into your local feature branch? Among other things, this will enable running tests via GitHub Actions.

I hadn't seen that in practice yet, it looks really nice.

  1. Add a test program under tests/performance that can be used to verify the performance gain. Even better would be if you can create an automated test under tests/system. I was thinking something like what we did for PR #34 where there was a pathological performance bug fixed and the test checked that the operation in question didn't take more than some small number of seconds. That might be overkill for this fix, though.

I put a test under tests/system using the example you gave. It creates a Scene with about 16k nodes, and then does 10.000 operations on them. This runs in around 2-2.5 seconds before the fix and in about 100-200 ms after. I put the cut-off point somewhere in the middle (800 ms).

I am planning to address the rest of the comments somewhere in the coming week.

Copy link
Member

@kevinrushforth kevinrushforth left a comment

The updated fix using registerChangeListener and the test look good. I left a few mostly-minor comments.

@hjohn
Copy link
Collaborator Author

@hjohn hjohn commented Feb 1, 2021

I think this change is complete now, including an integration test and tests on all new code. Please let me know if anything else needs to be done.

@arapte
Copy link
Member

@arapte arapte commented Feb 5, 2021

This PR also seems to fix another issue JDK-8259558. Test program is attached to the bug.

Time readings,
with this fix,
time to remove 100000 nodes : ~20 ms
time to add those removed 100000 nodes to a different parent: ~80 ms

without this fix,
time to remove 100000 nodes : ~1720 ms
time to add those removed 100000 nodes to a different parent: ~100 ms

Copy link
Member

@arapte arapte left a comment

Added couple minor comments and a suggestion.
Rest looks good to me, Observed no failures. It would be a great performance improvement for large scene applications.

@hjohn
Copy link
Collaborator Author

@hjohn hjohn commented Feb 6, 2021

This PR also seems to fix another issue JDK-8259558. Test program is attached to the bug.

Time readings,
with this fix,
time to remove 100000 nodes : ~20 ms
time to add those removed 100000 nodes to a different parent: ~80 ms

without this fix,
time to remove 100000 nodes : ~1720 ms
time to add those removed 100000 nodes to a different parent: ~100 ms

This is possible, as for each Node removed two listeners are also removed from the huge listener lists in Scene and Window (O(n) performance). Adding is not affected much as adding a listener to a huge list is not that costly.

arapte
arapte approved these changes Feb 15, 2021
Copy link
Member

@arapte arapte left a comment

Looks good to me.

Copy link
Member

@kevinrushforth kevinrushforth left a comment

The updated test and fix look good. While fixing up the copyright years you lost the initial year on one of the files, so that needs to be restored. I'll approve it after that.

Copy link
Member

@kevinrushforth kevinrushforth left a comment

Looks good.

@arapte will need to re-approve

arapte
arapte approved these changes Feb 18, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Feb 18, 2021

@hjohn 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:

8252935: Add treeShowing listener only when needed

Reviewed-by: kcr, arapte

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 16 new commits pushed to the master branch:

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.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@kevinrushforth, @arapte) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

@openjdk openjdk bot added the ready label Feb 18, 2021
@hjohn
Copy link
Collaborator Author

@hjohn hjohn commented Feb 20, 2021

/integrate

@openjdk openjdk bot added the sponsor label Feb 20, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Feb 20, 2021

@hjohn
Your change (at version fcaf2d6) is now ready to be sponsored by a Committer.

@kevinrushforth
Copy link
Member

@kevinrushforth kevinrushforth commented Feb 20, 2021

/sponsor

@openjdk openjdk bot closed this Feb 20, 2021
@openjdk openjdk bot added integrated and removed sponsor ready rfr labels Feb 20, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Feb 20, 2021

@kevinrushforth @hjohn Since your change was applied there have been 16 commits pushed to the master branch:

Your commit was automatically rebased without conflicts.

Pushed as commit 4733824.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
integrated
4 participants