Re: Possible approaches to JDK-8185886: Improve scrolling performance of TableView and TreeTableView
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 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 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:
Re: Possible approaches to JDK-8185886: Improve scrolling performance of TableView and TreeTableView
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
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 :
> >
> >> 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 :
> >>>
> 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.
Re: Possible approaches to JDK-8185886: Improve scrolling performance of TableView and TreeTableView
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 :
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 :
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
Re: Possible approaches to JDK-8185886: Improve scrolling performance of TableView and TreeTableView
Hi Jeanette,
Thanks for taking a look.
The PR was sort of inspired by JDK-8185886 (PR 108) because changes were
made there to make performance with long listener lists better (by using
a Map structure). I wanted to know the root cause of the long lists,
and found out that the long lists are on properties carried by Window
and Scene. This then was traced to the treeShowing property in Node
which registers listeners on Window and Scene.
I think Kevin then mentioned the JDK-8090322 issue which gave me further
background on why the treeShowing solution was introduced.
The change I proposed to fix this regression was also tested by the
author of PR 108 (Danny Gonzalez) and addressed the issue as well, see
this comment: https://github.com/openjdk/jfx/pull/108#issuecomment-615125904
I'm not aware of any other side effects of the change introduces with
JDK-8090322, but anything with lots of nodes in a single scene (in the
thousands) may see some performance degradation when adding/removing
nodes -- complex Tables or TreeTables are the primary culprit. It might
also affect the performance of hiding/showing a Window (when there are
many nodes), but that's not an action that is usually done a lot.
I did not include a JDK number -- I can include JDK-8185886 as it was
supposed to address that issue, or file a new issue referring both that
issue and JDK-8090322.
I donot think it will fix the performance issue with having many columns
in a table, although it may reduce its impact a bit. The issue there is
more that columns are not virtualized, so a table rotated 90 degrees
would exhibit the same problems as an unvirtualized table would because
of the sheer number of nodes involved.
--John
On 02/09/2020 14:24, 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 :
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 :
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.
Re: Possible approaches to JDK-8185886: Improve scrolling performance of TableView and TreeTableView
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 :
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 :
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
Re: Possible approaches to JDK-8185886: Improve scrolling performance of TableView and TreeTableView
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 :
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
Re: Possible approaches to JDK-8185886: Improve scrolling performance of TableView and TreeTableView
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 :
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
Re: Possible approaches to JDK-8185886: Improve scrolling performance of TableView and TreeTableView
It looks like those PR's are fixing different issues, and are mainly
complimentary. I think the challenge here is that issues like "bad
performance" are very generic and related to very specific use cases.
Hence, I think it would help to create more narrowed issues with a test
that quantitatively indicates there is a difference before/after applying
the PR. The difficulty of course is in the fact that a enhancement for a
specific case may cause regression for other cases.
- Johan
On Sun, Aug 30, 2020 at 7:02 PM Nir Lisker wrote:
> I didn't review these in detail, except for #1, which I think is worth it
> regardless, as it encompasses many more cases.
>
> If the authors of these can provide before and after benchmarks, we could
> also test what combinations of them are relevant. If we do #1, maybe others
> won't improve performance much.
>
> On Thu, Aug 27, 2020 at 3:18 AM Kevin Rushforth <
> kevin.rushfo...@oracle.com>
> wrote:
>
> > 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
> >
> >
>
Re: Possible approaches to JDK-8185886: Improve scrolling performance of TableView and TreeTableView
I didn't review these in detail, except for #1, which I think is worth it
regardless, as it encompasses many more cases.
If the authors of these can provide before and after benchmarks, we could
also test what combinations of them are relevant. If we do #1, maybe others
won't improve performance much.
On Thu, Aug 27, 2020 at 3:18 AM Kevin Rushforth
wrote:
> 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
>
>
Re: Possible approaches to JDK-8185886: Improve scrolling performance of TableView and TreeTableView
I just want to quickly mention that option 3
(https://github.com/openjdk/jfx/pull/185) is no longer a WIP -- all
functionality is working all tests pass.
The PR creates a TreeShowingExpression class which encapsulates the tree
showing logic that was in Node class. This class is then only used by
ProgressIndicatorSkin and PopupWindow. This effectively means that
instead of registering a listener on Window and Scene for all nodes
they're only registered for those classes that need to know the showing
status of a Scene.
--John
On 27/08/2020 02:17, Kevin Rushforth wrote:
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
Possible approaches to JDK-8185886: Improve scrolling performance of TableView and TreeTableView
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
Possible approaches to JDK-8185886: Improve scrolling performance of TableView and TreeTableView
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
