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 <nlis...@gmail.com> 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
> >
> >
>
