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

[Jeanette Winzenburg]

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
>>
>>
>>