On Thu, 6 Feb 2020 16:13:33 GMT, dannygonzalez <github.com+6702882+dannygonza...@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. 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. 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);`. 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. modules/javafx.base/src/main/java/com/sun/javafx/binding/ExpressionHelper.java line 194: > 192: > 193: private Map<InvalidationListener, Integer> invalidationListeners > = new LinkedHashMap<>(); > 194: private Map<ChangeListener<? super T>, Integer> changeListeners > = new LinkedHashMap<>(); Two comments on this: 1. The old implementation initialized these lazily, so we might want to preserve that behavior. I think it is reasonable since in most cases an observable won't have both types of listeners attached to it. 2. Since we're dealing wither performance optimizations here, we should think about what the `initialCapcity` and `loadFactor` of these maps should be, as it can greatly increase performance when dealing with a large amount of entries. ------------- PR: https://git.openjdk.java.net/jfx/pull/108
