RFR: 8252935: Add treeShowing listener only when needed
jhendrikx at openjdk.java.net
Wed Sep 9 21:35:57 UTC 2020
On Fri, 17 Apr 2020 17:18:00 GMT, John Hendrikx <jhendrikx at 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.
More information about the openjfx-dev