RFR: 8185886: Improve scrolling performance of TableView and TreeTableView

[Kevin Rushforth]

On Thu, 27 Aug 2020 00:07:21 GMT, Kevin Rushforth <kcr at 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:
>> ![TraceOpenJFX16ea1](https://user-images.githubusercontent.com/2043230/91322208-c2ba9900-e7bf-11ea-8918-cdbecd457cf2.png)
>>  
>> 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