On Wed, 26 Aug 2020 17:44:20 GMT, yosbits <github.com+7517141+yos...@openjdk.org> wrote:
>> 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. The https://github.com/openjdk/jfx/pull/185 is a full fix, not a WIP. It avoids registering the listeners on Scene and Window and moves the only uses of those listeners to their respective controls, PopupWindow and ProgressIndicatorSkin (the property involved is internal, so there is no risk of affecting 3rd parties). Please have a closer look. I updated the title to make it more clear it is no longer a WIP, unless someone has review comments. ------------- PR: https://git.openjdk.java.net/jfx/pull/108
