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