RFR: 8185886: Improve scrolling performance of TableView and TreeTableView

John Hendrikx jhendrikx at openjdk.java.net
Wed Aug 26 22:59:23 UTC 2020

On Wed, 26 Aug 2020 17:44:20 GMT, yosbits <github.com+7517141+yososs 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.
> 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

More information about the openjfx-dev mailing list