RFR: 8252936: Optimize removal of listeners from ExpressionHelper.Generic

[John Hendrikx]

On Thu, 16 Apr 2020 06:34:39 GMT, yosbits <github.com+7517141+yososs at openjdk.org> wrote:

>> @hjohn 
>> I haven't quantified exactly where all the listeners are coming from but the Node class for example has various listeners on it.
>> 
>> The following changeset  (which added an extra listener on the Node class) impacted TableView performance significantly (although it was still bad before this change in my previous tests):
>> 
>>> commit e21606d3a1b73cd4b44383babc750a4b4721edfd
>>> Author: Florian Kirmaier <fk at sandec.de<mailto:fk at sandec.de>>
>>> Date:   Tue Jan 22 09:08:36 2019 -0800
>>> 
>>>     8216377: Fix memoryleak for initial nodes of Window
>>>     8207837: Indeterminate ProgressBar does not animate if content is added after scene is set on window
>>> 
>>>     Add or remove the windowShowingChangedListener when the scene changes
>> 
>> As you can imagine, a TableView with many columns and rows can be made up of many Node classes. The impact of having multiple listeners deregistering on the Node when new rows are being added to a TableView can be a significant performance hit on the JavaFX thread. 
>> 
>> I don't have the time or knowledge to investigate why these listeners are all needed especially around the Node class. I went directly to the source of the problem which was the linear search to deregister each listener.
>> 
>> I have been running my proposed fix in our JavaFX application for the past few months and it has saved it from being unusable due to the JavaFx thread being swamped.
>
> The patch proposed here does not share the case where the listener deletion performance becomes a bottleneck.
> 
> I think that it is necessary to reproduce it by testing first, but
> 
> If you just focus on improving listener removal performance,
> 
> If the lifespan of a large number of registered listeners is biased,
> It seems like the next simple change can improve delete performance without changing the data structure.
> 
> * Change the search from the front of the list to the search from the back.
> 
> This will reduce the number of long-life listeners matching.

Looking at the commit  https://github.com/openjdk/jfx/commit/e21606d3a1b73cd4b44383babc750a4b4721edfd 
 it seems that the long listener lists are actually part of the `Scene`'s `Window` property and the `Window`'s `Showing` property.  Each `Node` registers itself on those and so the listener lists for those properties would scale with the number of nodes.

A test case showing this problem would really be great as then the patch can also be verified to solve the problem, but I suppose it could be reproduced simply by having a large number of Nodes in a scene.  @dannygonzalez could you give us an idea how many Nodes we're talking about?  1000? 10.000?

It also means there might be other options, do Nodes really need to add these listeners and for which functionality and are there alternatives?  It would also be possible to target only these specific properties with an optimized listener list to reduce the impact of this change.

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

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