On Fri, 17 Apr 2020 17:18:00 GMT, John Hendrikx <jhendr...@openjdk.org> wrote:
>> These listeners exist for a good reason. Removing them will almost certainly >> cause regressions in behavior. See >> [JDK-8090322](https://bugs.openjdk.java.net/browse/JDK-8090322) as well as >> the follow-on fixes (since that was a rather >> involved fix in the first place). > > @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. @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. ------------- PR: https://git.openjdk.java.net/jfx/pull/185
