On Wed, 26 Aug 2020 14:08:37 GMT, dannygonzalez <github.com+6702882+dannygonza...@openjdk.org> wrote:
>> @hjohn, agreed regards the issues of adding a listener to each node. >> >> Would it be worth doing the additional work of updating PopupWindow and >> ProgressIndicatorSkin to add their own >> listeners to make this a pull request that can be reviewed officially? >> I await any further comments from @kevinrushforth et al. > > 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. ------------- PR: https://git.openjdk.java.net/jfx/pull/108
