RFR: 8252935: Add treeShowing listener only when needed

[John Hendrikx]

On Fri, 17 Apr 2020 23:47:42 GMT, John Hendrikx <jhendrikx at openjdk.org> wrote:

>> @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.

@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.

-------------

PR: https://git.openjdk.java.net/jfx/pull/185