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
