On Thu, 27 Aug 2020 00:07:21 GMT, Kevin Rushforth <k...@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. > >> 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). > > Given PR #185, which was mentioned above, (it isn't out for review yet, but I > want to evaluate it), this would be a 4th > approach. > As long as this really doesn't introduce a leak, it seems promising. > > I note that these are not mutually exclusive. > > We should discuss this on the list and not just in one or more of of the 3 > (soon to be 4) open pull requests. @dannygonzalez Per [this message](https://mail.openjdk.java.net/pipermail/openjfx-dev/2020-September/027534.html) on the openjfx-dev mailing list, I have filed a new JBS issue for this PR to use. Please change the title to: 8252936: Optimize removal of listeners from ExpressionHelper.Generic ------------- PR: https://git.openjdk.java.net/jfx/pull/108
