Possible approaches to JDK-8185886: Improve scrolling performance of TableView and TreeTableView

[John Hendrikx]

The "dynamic update performance" performance issue we are seeing is a 
regression from JDK-8090322.  In this change the Node treeShowing 
property was introduced.  The JDK-8090322 warns in its description about:

"""    Considerations:
* This is too expensive to calculate for all nodes by default. So the 
simplest way to provide it would be a special binding implementation or 
a util class. Where you create a instance and pass in the node you are 
interested in. It can then register listeners all the way up the tree 
and listen to what it needs.  """

The above comment is warning against the fact that registering listeners 
for EACH Node on Window and Scene is going to be a performance issue. 
As nodes can number in the 1000's or 10.000's, that's a lot of listeners 
to store in a List data structure.

PR 185 targets this issue and implements the feature in JDK-8090322 in
the way that was suggested in the above comment, instead of how it 
currently is implemented (registering listeners for every Node, just in 
case someone needs the treeShowing property).

It's scope is not as broad as it would seem (because of a change in 
Node).  It effectively only makes a small change in two controls 
(PopupWindow and ProgressIndicatorSkin).

--John



On 31/08/2020 16:54, Jeanette Winzenburg wrote:
>
> Looking at the examples provided in 108/125: apart from both having many
> columns (> 300 makes them really nasty) they differ in
>
> Table content:
> 125 - static data
> 108 - items are frequently modified (added)
>
> Perceived performance:
> 125 - vertical scrolling: thumb/content lags behind mouse
> 108 - with enough columns, all interaction is sluggish (mouse, keys,
> ..), scrolling being just one of them
>
> Both have examples, running those against the suggested fixes (with 108a
> for Jose's approach)
> 125 - fixes its own example, does nothing for the other
> 108, 108a, 185 - improves its own example, does nothing for other
>
> So we seem to have multiple issues that are (mostly) orthogonal: one
> being the broken/missing horizontal virtualization (125), the other
> related to dynamic update of table content (108, 108a, 185). We need to
> solve both, the solution/s for one looks (mostly?) unrelated to the
> solution to the other.
>
> 125 is the only one PR for its use-case, and it seems to do its job.
> From those targeting the dynamic data update all except Jose's (not yet
> formalized into a PR) approach feel too broad: table's reaction to items
> modifications is .. suboptimal in more than one aspect. Changing overall
> notification implementation to improve that, sounds like covering it up.
>
> -- Jeanette
>
> Zitat von Kevin Rushforth <kevin.rushforth at oracle.com>:
>
>> Sorry for the badly formatted html. Here it is again.
>>
>> I see some progress being made on the {Tree}TableView performance
>> issue. To summarize where I think we are:
>>
>> There are currently 2 different approaches under review:
>>
>> 1. https://github.com/openjdk/jfx/pull/108 : optimization in
>> javafx.base to make removing listeners faster
>> 2. https://github.com/openjdk/jfx/pull/125 : optimization in TableView
>> to reduce the number of add / removes
>>
>> In addition, the following is a WIP PR that the author thinks could be
>> a 3rd approach:
>>
>> 3. https://github.com/openjdk/jfx/pull/185 : optimization in scene
>> graph to avoid install treeShowing listeners on Window and Scene for
>> all nodes
>>
>> Jose has proposed a 4th approach as a comment to PR #108, and as I
>> understand it, he will propose a PR shortly.
>>
>> 4. Don't clear the list of children in a VirtualFlow when the number
>> of items changes.
>>
>> So the first thing that is needed is to evaluate the approaches and
>> decide which one to pursue.
>>
>> Options 1 and 3 are more broad in their scope, option #2 is more
>> targeted (to TableView), but within that area is a larger change.
>> Option #3 would remove the (internal) treeShowing property as a
>> generally available concept and only use it for controls like
>> ProgressIndicator that really need it. Option #4 affects only controls
>> that use VirtualFlow (ListView, TableVIew, TreeTableView), and seems
>> not to be a large change (presuming we can verify that no leak is
>> introduced).
>>
>> I note that these fixes are not mutually exclusive, but I do think we
>> need to settle on a primary approach and use that to fix this issue.
>> If there are still performance problems after that fix, we can
>> consider one (or more) of the others.
>>
>> Comments?
>>
>> -- Kevin
>
>
>