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

[dannygonzalez]

On Tue, 14 Apr 2020 12:20:10 GMT, John Hendrikx <jhendrikx at openjdk.org> wrote:

>>> Hmm .. personally, I would consider changing such a basic (and probably highly optimized) implementation used all over the framework is a very high risk change that at the worst outcome would detoriate internal and external code. So before going that lane, I would try - as you probably have, just me not seeing it :) - to tackle the problem from the other end:
>>> 
>>> > I know that in our application, the **thousands of listeners** unregistering when using a TableView was stalling the JavaFX thread.
>>> 
>>> .. sounds like a lot. Any idea, where exactly they come into play?
>> 
>> I did start to look at why there were so many change listeners unregistering but felt that would take a deeper understanding of the architecture and design decisions of JavaFX scene graph and I didn't have that time to invest. 
>> I do know that there are thousands of them unregistering in a TableView and unregistering them is a bottleneck for the JavaFX thread.
>> 
>> There are multiple change listeners on a Node for example, so you can imagine with the number of nodes in a TableView this is going to be a problem if the unregistering is slow.
>> 
>> To get our application usable I profiled the code and saw this unregistering as a major bottleneck, hence why I looked at this more obvious solution.
>> 
>> I'm happy to look at the problem from the other angle and happy to listen to suggestions from people with more experience of the design and architecture but tackling the problem from the perspective of re-architecting the behaviour of listeners in the scene graph would be more work than I could feasibly take on.
>
> @dannygonzalez You mentioned "There are multiple change listeners on a Node for example, so you can imagine with the number of nodes in a TableView this is going to be a problem if the unregistering is slow.".
> 
> Your changes in this PR seem to be focused on improving performance of unregistering listeners when there are thousands of listeners listening on changes or invalidations of the **same** property.  Is this actually the case?  Is there a single property in TableView or TreeTableView where such a high amount of listeners are present?  If so, I think the problem should be solved there.
> 
> As it is, I donot think this PR will help unregistration performance of listeners when the listeners are  spread out over dozens of properties, and although high in total number, the total listeners per property would be low and their listener lists would be short.  Performance of short lists (<50 entries) is almost unbeatable, so it is relevant for this PR to know if there are any properties with long listener lists.

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

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

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