JDK-8193445 Performance Test

[Dean Wookey]

Hi,

I just wanted to clarify a few things about the fix. The test I showed
hints at a potentially safer way to implement the fix for this problem. One
would expect adding nodes 1 by 1 to the scene graph to be less efficient
than adding them all at once, but it's far worse to add them all at once.
The fix demonstrates that the test's performance problem comes from the
reapplication of css, and is not necessarily the best fix for the problem,
it's just the least impactful fix I could come up with, since the idea is
that adding nodes one by one is safe already.

Reapplying CSS to a Node but not its children could cause a problem if
> there are styles in the parent or the parent's parents that affect the
> children. It seems like bypassing children in reapplyCSS is bound to
> cause a regression.
>

CSS does get applied to the children. The problem was that each parent
would call scenesChanged, which would then call scenesChanged on all the
children recursively. As they're bubbling out they call reapplyCSS on
themselves which calls reapplyCss on themselves and all their children.
Jonathan's solution was to only allow the node that was added to the scene
graph to reapplyCSS to its children, thus applying css in a top down way. I
suspect with some modifications it could work. One thing it didn't do was
call reapplyCSS (different from reapplyCss) on every node. It only called
reapplyCss on every node, and there is some additional logic in reapplyCSS.
(The commit which undoes this change is here:
https://github.com/javafxports/openjdk-jfx/commit/cd14f72776281a2384c50a35e618f1fb258c9430#diff-4a65018af7ebb15827ff1c67a3c29e86
).

In mine, reapplyCSS happens first and doesn't propagate down to the
children. Instead, the scenesChanged method will trigger invalidatedScenes
on all the children, and they will reapplyCSS for themselves. Basically
each node calls reapplyCSS for itself, then propagates. In this way css is
applied top down in a way similar to adding the nodes 1 by 1. Also, only
this one case where nodes are added all at once is affected.

Kevin has also mentioned https://bugs.openjdk.java.net/browse/JDK-8173301
as a way to mitigate the problem. I think something like that would be fine
for this particular css performance issue, but there are several other
issues. It would be nice to have a safer way to work on these issues so
that they could get tackled.

I think there are several options here. I just wanted to share some
insights, and perhaps contribute a test or two. Kevin mentioned "We will
need a performance regression test to measure the gain." Are there any
performance tests like this already I could look at as an example?

Dean



On Wed, Nov 7, 2018 at 3:37 PM David Grieve <david.grieve at oracle.com> wrote:

> One of the dangers of mucking around with the CSS code is whether or not
> the changes break things like popups, dialogs, menus. And whether or not
> the change breaks inline styles versus attributes set in code, versus
> stylesheets added to the scene/subscene/control, versus default
> stylesheets. And whether or not the changes breaks skins for controls.
>
> Reapplying CSS to a Node but not its children could cause a problem if
> there are styles in the parent or the parent's parents that affect the
> children. It seems like bypassing children in reapplyCSS is bound to
> cause a regression.
>
> Also, because a new scene root could affect styles, CSS is reapplied
> after calling sceneChanged - your change puts it before, so I question
> whether or not this will cause a regression. I think if you skip
> reapplying CSS when a Node's scene has changed, then the children won't
> get the correct styles, and this will affect layout.
>
> I haven't spent much time evaluating this change in detail, but I'm
> doubtful that it won't cause regressions.
>
>