Possible approaches to JDK-8185886: Improve scrolling performance of TableView and TreeTableView
Kevin Rushforth
kevin.rushforth at oracle.com
Tue Sep 8 20:12:17 UTC 2020
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.rushforth at oracle.com <mailto:kevin.rushforth at 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 <hjohn at xs4all.nl <mailto: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
> <mailto: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
> <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
> >>>
> >>>
> >>>
> >
> >
> >
>
>
>
> --
>
More information about the openjfx-dev
mailing list