Thanks for filing it, Jose.
I think it's better not to use JDK-8185886 for any of these PRs, since
it's too generic a description, and was meant as an umbrella issue
anyway, so I closed it as a duplicate of the 4 issues that are split out
from it.
I filed a new issue for each of PR #108 and PR #185.
There is already an issue about the lack of virtualization in the
horizontal direction, JDK-8185887, so we can use that for PR #125.
Here is the list of the 4 PRs under review:
PR #108 [1] : JDK-8252936 [2] : Optimize removal of listeners from
ExpressionHelper.Generic
PR #125 [3] : JDK-8185887 [4] : TableRowSkinBase fails to correctly
virtualize cells in horizontal direction
PR #185 [5] : JDK-8252935 [6] : Add treeShowing listener only when needed
PR #298 [7] : JDK-8252811 [8] : The list of cells in a VirtualFlow is
cleared every time the number of items changes
For the first three PRs, I ask the author of the PR to update the title
of their PR to match their associated JBS issue.
We can proceed to discuss each fix in their respective PRs.
-- Kevin
[1] https://github.com/openjdk/jfx/pull/108
[2] https://bugs.openjdk.java.net/browse/JDK-8252936
[3] https://github.com/openjdk/jfx/pull/125
[4] https://bugs.openjdk.java.net/browse/JDK-8185887
[5] https://github.com/openjdk/jfx/pull/185
[6] https://bugs.openjdk.java.net/browse/JDK-8252935
[7] https://github.com/openjdk/jfx/pull/298
[8] https://bugs.openjdk.java.net/browse/JDK-8252811
On 9/4/2020 9:04 AM, José Pereda wrote:
I've filed https://bugs.openjdk.java.net/browse/JDK-8252811 (under
javafx/controls).
I believe this is not an alternative to any of the other three
issues, but obviously a less invasive one, as it only implies changes
in VirtualFlow.
Once tackled, it should directly increase performance and reduce CPU
usage of TableView/TreeTableView/ListView controls when their items
change frequently.
But it will also benefit from the improvements of the other three
approaches.
Jose
On Fri, Sep 4, 2020 at 1:46 AM Kevin Rushforth
<kevin.rushfo...@oracle.com <mailto:kevin.rushfo...@oracle.com>> wrote:
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 <hj...@xs4all.nl <mailto:hj...@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.rushfo...@oracle.com
<mailto:kevin.rushfo...@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
<https://urldefense.com/v3/__https://github.com/openjdk/jfx/pull/108__;!!GqivPVa7Brio!KKyin4SuSWheT-b4caAsZF_NoXktXLzN4u06UJjtC7VRkTGdedov8ZVfFVGL4ViqWALw$>
: optimization in
>>>> javafx.base to make removing listeners faster
>>>> 2. https://github.com/openjdk/jfx/pull/125
<https://urldefense.com/v3/__https://github.com/openjdk/jfx/pull/125__;!!GqivPVa7Brio!KKyin4SuSWheT-b4caAsZF_NoXktXLzN4u06UJjtC7VRkTGdedov8ZVfFVGL4S6OPoyp$>
: 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
<https://urldefense.com/v3/__https://github.com/openjdk/jfx/pull/185__;!!GqivPVa7Brio!KKyin4SuSWheT-b4caAsZF_NoXktXLzN4u06UJjtC7VRkTGdedov8ZVfFVGL4dXd0a_r$>
: 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
>>>
>>>
>>>
>
>
>
--
