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

[Kevin Rushforth]

It seems clear now that we will need 3 different JBS issues for these 
proposed performance enhancements. It's a holiday weekend coming up in 
the US, so I can file the other two issues unless someone else gets to 
it first. Unless there is a good reason to do otherwise, I propose:

The JBS Issue associated with PR #108 should be filed against javafx/base
The JBS Issue associated with PR #125 should be filed against 
javafx/controls (or we can reuse JDK-8185886)
The JBS Issue associated with PR #185 should be filed against 
javafx/scenegraph

Jose: Is you approach an alternative to one of the above or could it be 
considered a 4th approach? If the latter, can you file a new JBS Issue 
for that?

-- Kevin


On 9/2/2020 5:24 AM, Jeanette Winzenburg wrote:
>
> Hi John,
>
> thanks for the clarification :)
>
> Hmm .. but then it's not really a PR against JDK-8185886 (scrolling 
> performance was always bad with many columns) but against - yet to be 
> reported - side-effect of JDK-8090322 which happens to detoriate 
> tableView performance even further (there might be other side-effects)?
>
> -- Jeanette
>
> Zitat von John Hendrikx <hjohn at xs4all.nl>:
>
>> 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
>>>
>>>
>>>
>
>
>