On Wed, 26 Aug 2020 15:40:57 GMT, Jose Pereda <jper...@openjdk.org> wrote:
>> I have attached a code sample. If you use OpenJFX 16-ea+1 and run visual VM >> and look at the hotspots in the JavaFX >> thread, you can see that about 45% of the time in the JavaFX thread is spent >> in removeListener calls. >> Note: In CPU settings of VisualVM, I removed all packages from the "Do not >> profile packages section". >> >> [JavaFXSluggish.java.zip](https://github.com/openjdk/jfx/files/5130298/JavaFXSluggish.java.zip) > > So far, there are two alternative fixes for the bad performance issue while > scrolling TableView/TreeTableViews: > > - this one #108, that tries to improve the performance of excessive number of > `removeListener` calls > - the WIP #185 that avoids registering two listeners (on Scene and on Window) > for each and every Node. > > For the case presented, where new items are constantly added to the > TableView, the trace of calls that reach > `com.sun.javafx.binding.ExpressionHelper.removeListener()` is something like > this: >  > > As can be seen, all those calls are triggered by the change of the number of > cells in > [VirtualFlow::setCellCount](https://github.com/openjdk/jfx/blob/master/modules/javafx.controls/src/main/java/javafx/scene/control/skin/VirtualFlow.java#L804). > Whenever the cell count changes there is this > [call](https://github.com/openjdk/jfx/blob/master/modules/javafx.controls/src/main/java/javafx/scene/control/skin/VirtualFlow.java#L843): > sheetChildren.clear(); > > This happens every time the number of items in the `TableView` changes, as > the `VirtualFlow` keeps track of the number > of virtual cells (`cellCount` is the total number of items) while the number > of actual cells or number of visible nodes > used is given by `sheetChildren.size()`. This means that all the actual > cells (nodes) that are used by the > `VirtualFlow` are disposed and recreated all over again every time the number > of items changes, triggering all those > calls to unregister those nodes from the scene that ultimately lead to remove > the listeners with > `ExpressionHelper.removeListener`. However, this seems unnecessary, as the > number of actual cells/nodes doesn't change > that much, and causes this very bad performance. On a quick test over the > sample posted, just removing that line gives > a noticeable improvement in performance.. > There is a concern though due to the > [comment](https://github.com/openjdk/jfx/blob/master/modules/javafx.controls/src/main/java/javafx/scene/control/skin/VirtualFlow.java#L839): > // Fix for RT-13965: Without this line of code, the number of items in > // the sheet would constantly grow, leaking memory for the life of the > // application. This was especially apparent when the total number of > // cells changes - regardless of whether it became bigger or smaller. > sheetChildren.clear(); > > There are some methods in VirtualFlow that already manage the lifecycle of > this list of nodes (clear, remove cells, add > cells, ...), so I don't think that is the case anymore for that comment: the > number of items in the sheet doesn't > constantly grow and there is no memory leak. Running the attached sample for > a long time, and profiling with VisualVM, > shows improved performance (considerable drop in CPU usage), and no issues > regarding memory usage. > So I'd like to propose this third alternative, which would affect only > VirtualFlow and the controls that use it, but > wouldn't have any impact in the rest of controls as the other two options (as > `ExpressionHelper` or `Node` listeners > wouldn't be modified). Thoughts and feedback are welcome. I confirmed the sample code (JavaFX Sluggish), This is not scroll performance It seems to reproduce the additional performance issue. Therefore, it is not considered appropriate as a fix for JDK-8185886. I know you are reproducing another performance issue, but I'm proposing to fix scrolling performance issues in #125. ------------- PR: https://git.openjdk.java.net/jfx/pull/108
