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
