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

[dannygonzalez]

On Sun, 23 Feb 2020 01:05:25 GMT, Nir Lisker <nlisker at openjdk.org> wrote:

>> https://bugs.openjdk.java.net/browse/JDK-8185886
>> 
>> Optimisation to ExpressionHelper.Generic class to use Sets rather than Arrays to improve listener removal speed.
>> 
>> This was due to the removal of listeners in TableView taking up to 50% of CPU time on the JavaFX Application thread when scrolling/adding rows to TableViews.
>> 
>> This may alleviate some of the issues seen here:
>> 
>> TableView has a horrific performance with many columns #409
>> https://github.com/javafxports/openjdk-jfx/issues/409#event-2206515033
>> 
>> JDK-8088394 : Huge memory consumption in TableView with too many columns
>> JDK-8166956: JavaFX TreeTableView slow scroll performance
>> JDK-8185887: TableRowSkinBase fails to correctly virtualise cells in horizontal direction
>> 
>> OpenJFX mailing list thread: TableView slow vertical scrolling with 300+ columns
>> https://mail.openjdk.java.net/pipermail/openjfx-dev/2020-January/024780.html
>
> modules/javafx.base/src/main/java/com/sun/javafx/binding/ExpressionHelper.java line 283:
> 
>> 281:             // while the observers are being notified is safe
>> 282:             final Map<InvalidationListener, Integer> curInvalidationList = new LinkedHashMap<>(invalidationListeners);
>> 283:             final Map<ChangeListener<? super T>, Integer> curChangeList = new LinkedHashMap<>(changeListeners);
> 
> You only need the entry set, so you don't need to copy the map, just the set.

Thanks, yes the EntrySet would make more sense here. I'll fix that up.

> modules/javafx.base/src/main/java/com/sun/javafx/binding/ExpressionHelper.java line 285:
> 
>> 283:             final Map<ChangeListener<? super T>, Integer> curChangeList = new LinkedHashMap<>(changeListeners);
>> 284: 
>> 285:             curInvalidationList.entrySet().forEach(entry -> fireInvalidationListeners(entry));
> 
> The lambda can be converted to a method reference.

Agreed,  I'll fix.

> modules/javafx.base/src/main/java/com/sun/javafx/binding/ExpressionHelperBase.java line 64:
> 
>> 62:                 ((WeakListener)t).wasGarbageCollected();
>> 63: 
>> 64:         listeners.entrySet().removeIf(e -> p.test(e.getKey()));
> 
> This can be `listeners.keySet().removeIf(p::test);`.

Agreed, will change.

> modules/javafx.base/src/main/java/com/sun/javafx/binding/ExpressionHelper.java line 197:
> 
>> 195:         private T currentValue;
>> 196:         private int weakChangeListenerGcCount = 2;
>> 197:         private int weakInvalidationListenerGcCount = 2;
> 
> Why are these set to 2 and why do you need them at all? The previous implementation needed to grow and shrink the array so it had to keep these, but `Map` takes care of this for you.

I agree, I kept these in as I wasn't sure if there was a need to manually force the garbage collection of weak listeners at the same rate as the original implementation.
Removing this would make sense to me also.

Updated my thoughts on this, see my comments below.

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

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