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

[John Hendrikx]

On Fri, 17 Apr 2020 07:22:20 GMT, dannygonzalez <github.com+6702882+dannygonzalez at openjdk.org> wrote:

>> I've tested this pull request locally a few times, and the performance improvement is quite significant.   A test with 20.000 nested stack panes resulted in these average times:
>> 
>> - Add all 51 ms
>> - Remove all 25 ms
>> 
>> Versus the unmodified code:
>> 
>> - Add all 34 ms
>> - Remove all 135 ms
>> 
>> However, there are some significant functional changes as well that might impact current users:
>> 
>> 1. The new code ensures that all listeners are notified even if the list is modified during iteration by always making a **copy** when an event is fired.  The old code only did so when it was **actually** modified during iteration.  This can be mitigated by making the copy in the code that modifies the list (as the original did) using the `locked` flag to check whether an iteration was in progress.  
>> 
>> 2. There is a significant increase in memory use.  Where before each listener occupied an entry in an array, now each listener is wrapped by `Map.Entry` (the Integer instance used per entry can be disregarded).  I estimate around 4-8x more heap will be consumed (the numbers are small however, still well below 1 MB for 20.000 entries).  If this is an issue, a further level could be introduced in the listener implementation hierarchy (singleton -> array -> map).
>> 
>> 3. Even though the current version of this pull request takes care to notify duplicate listeners the correct amount of times, it does not notify them in the correct order with respect to other listeners.  If one registers listeners (a, b, a) the notification order will be (a, a, b).
>> 
>> The last point is not easily solved and could potentially cause problems.
>> 
>> Finally I think this solution, although it performs well is not the full solution.  A doubling or quadrupling of nodes would again run into serious limitations.  I think this commit https://github.com/openjdk/jfx/commit/e21606d3a1b73cd4b44383babc750a4b4721edfd should not have introduced another listener for each Node on the Window class.  A better solution would be to only have the Scene listen to Window and have Scene provide a new combined status property that Node could use for its purposes.
>> 
>> Even better however would be to change the properties involved to make use of the hierarchy naturally present in Nodes, having child nodes listen to their parent, and the top level node listen to the scene.  This would reduce the amount of listeners on a single property in Scene and Window immensely, instead spreading those listeners over the Node hierarchy, keeping listener lists much  shorter, which should scale a lot better.
>
> @hjohn 
> 
>> 2. There is a significant increase in memory use. Where before each listener occupied an entry in an array, now each listener is wrapped by Map.Entry (the Integer instance used per entry can be disregarded). I estimate around 4-8x more heap will be consumed (the numbers are small however, still well below 1 MB for 20.000 entries). If this is an issue, a further level could be introduced in the listener implementation hierarchy (singleton -> array -> map).
> 
> There was discussion about lazy initialisation of the LinkedHashMap when needed and/or have some sort of strategy where we could use arrays or lists if the number of listeners are less than some threshold (i.e. introducing another level to the hierarchy as you mentioned).
> This was mentioned here also: https://github.com/openjdk/jfx/pull/108#issuecomment-590838942

I've implemented an alternative solution: Removing the listeners on `Window.showingProperty` and `Scene.windowProperty` completely.  They are in fact only used in two places: `PopupWindow` in order to remove itself if the `Window` is no longer showing, and `ProgressIndicatorSkin`.  These two can be easily replaced with their own listeners for these properties instead of burdening **all** nodes with these listeners only to support these two classes.

I left the `isTreeShowing` method in, and implemented it simply as `isTreeVisible() && isWindowShowing()` as that's really the only difference between "visible" and "showing" apparently.

Here is the test result with 20.000 nested StackPanes with only this change in:

- Add all 45 ms
- Remove all 25 ms

I think this might be a good solution as it completely avoids these listeners.

@dannygonzalez I added a proof of concept here if you want to play with it: https://github.com/openjdk/jfx/pull/185

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

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