JDK-8193445 Performance Test

[Dean Wookey]

Hi,

I was going to ask if it was possible to reopen JDK-8151756 (
https://bugs.openjdk.java.net/browse/JDK-8151756) since it was fixed but
reverted in JDK-8183100 (https://bugs.openjdk.java.net/browse/JDK-8183100)
after causing several regressions. I only noticed now that a followup bug
was created: JDK-8193445 which reopens the issue.

The code below demonstrates the problem where adding nodes to the scene
graph all at once performs exponentially slower than adding them one by
one. I get the following results with jfx11.0.1:

One by one: 138ms
All at once: 16704ms

I've made a potential fix, different to the one tried previously which
applies the css as if the nodes were being added one by one:
https://github.com/DeanWookey/openjdk-jfx/commit/65a1ed82bce262294f1969e9a12e1126ec8a1ec6

It passes the main tests, as well as the systemTest JDK8183100Test.java
from https://bugs.openjdk.java.net/browse/JDK-8193494.

This is probably not a suitable issue to work on for a first time
contributor, but perhaps I could work on a performance test if someone can
point me in the direction of existing performance tests?

Dean

package applycsstest;

import javafx.application.Application;
import javafx.application.Platform;
import javafx.scene.Scene;
import javafx.scene.layout.StackPane;
import javafx.stage.Stage;

/**
*
* @author Dean
*/
public class ApplyCssTest extends Application {

@Override
public void start(Stage primaryStage) {
System.out.println("One by one: " + addToSceneOneByOne() + "ms");
System.out.println("All at once: " + addToSceneAllAtOnce() + "ms");
Platform.exit();
}

public static void main(String[] args) {
launch(args);
}

public long addToSceneOneByOne() {
StackPane root = new StackPane();
Scene scene = new Scene(root, 300, 250);

long start = System.currentTimeMillis();
StackPane firstChild = new StackPane();
root.getChildren().add(firstChild); //add to the scene graph first
addNodesOneByOne(1000, firstChild); //then add children one by one
return System.currentTimeMillis() - start;
}

public long addToSceneAllAtOnce() {
StackPane root = new StackPane();
Scene scene = new Scene(root, 300, 250);

long start = System.currentTimeMillis();
StackPane firstChild = new StackPane();
addNodesOneByOne(1000, firstChild); //build the node up, then
root.getChildren().add(firstChild); //add to scene graph all at once
return System.currentTimeMillis() - start;
}

public void addNodesOneByOne(int numToAdd, StackPane parent) {
StackPane last = parent;
for (int i = 0; i < numToAdd; i++) {
StackPane curr = new StackPane();
last.getChildren().add(curr);
last = curr;
}
}
}

[David Grieve]

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.

Post by Dean Wookey
Hi,
I was going to ask if it was possible to reopen JDK-8151756 (
https://bugs.openjdk.java.net/browse/JDK-8151756) since it was fixed but
reverted in JDK-8183100 (https://bugs.openjdk.java.net/browse/JDK-8183100)
after causing several regressions. I only noticed now that a followup bug
was created: JDK-8193445 which reopens the issue.
The code below demonstrates the problem where adding nodes to the scene
graph all at once performs exponentially slower than adding them one by
One by one: 138ms
All at once: 16704ms
I've made a potential fix, different to the one tried previously which
https://github.com/DeanWookey/openjdk-jfx/commit/65a1ed82bce262294f1969e9a12e1126ec8a1ec6
It passes the main tests, as well as the systemTest JDK8183100Test.java
from https://bugs.openjdk.java.net/browse/JDK-8193494.
This is probably not a suitable issue to work on for a first time
contributor, but perhaps I could work on a performance test if someone can
point me in the direction of existing performance tests?
Dean
package applycsstest;
import javafx.application.Application;
import javafx.application.Platform;
import javafx.scene.Scene;
import javafx.scene.layout.StackPane;
import javafx.stage.Stage;
/**
*
*/
public class ApplyCssTest extends Application {
@Override
public void start(Stage primaryStage) {
System.out.println("One by one: " + addToSceneOneByOne() + "ms");
System.out.println("All at once: " + addToSceneAllAtOnce() + "ms");
Platform.exit();
}
public static void main(String[] args) {
launch(args);
}
public long addToSceneOneByOne() {
StackPane root = new StackPane();
Scene scene = new Scene(root, 300, 250);
long start = System.currentTimeMillis();
StackPane firstChild = new StackPane();
root.getChildren().add(firstChild); //add to the scene graph first
addNodesOneByOne(1000, firstChild); //then add children one by one
return System.currentTimeMillis() - start;
}
public long addToSceneAllAtOnce() {
StackPane root = new StackPane();
Scene scene = new Scene(root, 300, 250);
long start = System.currentTimeMillis();
StackPane firstChild = new StackPane();
addNodesOneByOne(1000, firstChild); //build the node up, then
root.getChildren().add(firstChild); //add to scene graph all at once
return System.currentTimeMillis() - start;
}
public void addNodesOneByOne(int numToAdd, StackPane parent) {
StackPane last = parent;
for (int i = 0; i < numToAdd; i++) {
StackPane curr = new StackPane();
last.getChildren().add(curr);
last = curr;
}
}
}

[Scott Palmer]

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

I agree that it is necessary to re-apply CSS to child nodes when the parent node CSS changes. The issue that I originally reported was that I could see that child nodes had CSS re-applied more than once. The number of times CSS is applied is currently proportional to how deep in the scene graph hierarchy the node is.

When a subtree is added to the scene graph, I would expect that CSS can be reapplied to each node only once, so long as it is done in a top-down manner.

I could be wrong, I’m not certain how rules relating to sibling nodes might invalidate that assumption. In HTML there is the concept of adjacent sibling combinator, e.g. “img + p" for paragraphs that come immediately after any image. I don’t think JavaFX has anything like that thoughts o top-down should work out.

Scott

[Kevin Rushforth]

My thoughts as well. I would love to see a safe robust fix for this
issue, but there is a very real possibility of a regression: We thought
we had a safe fix earlier and only later discovered the (multiple)
regressions a few months after it was released.

One possible way to improve performance is to allow applications to
manage when it is safe to skip applying CSS to certain nodes under app
control, as proposed in
https://bugs.openjdk.java.net/browse/JDK-8173301. I'm not sure that
would be as effective, but at least it would only affect apps that "opt
in" and only for those parts of the scene graph that are so designated.

I do like the idea of coming up with better tests that can be used to
validate any potential future fix.

-- Kevin

Post by David Grieve
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.

Post by Dean Wookey
Hi,
I was going to ask if it was possible to reopen JDK-8151756 (
https://bugs.openjdk.java.net/browse/JDK-8151756) since it was fixed but
reverted in JDK-8183100
(https://bugs.openjdk.java.net/browse/JDK-8183100)
after causing several regressions. I only noticed now that a followup bug
was created: JDK-8193445 which reopens the issue.
The code below demonstrates the problem where adding nodes to the scene
graph all at once performs exponentially slower than adding them one by
One by one: 138ms
All at once: 16704ms
I've made a potential fix, different to the one tried previously which
https://github.com/DeanWookey/openjdk-jfx/commit/65a1ed82bce262294f1969e9a12e1126ec8a1ec6
It passes the main tests, as well as the systemTest JDK8183100Test.java
from https://bugs.openjdk.java.net/browse/JDK-8193494.
This is probably not a suitable issue to work on for a first time
contributor, but perhaps I could work on a performance test if someone can
point me in the direction of existing performance tests?
Dean
package applycsstest;
import javafx.application.Application;
import javafx.application.Platform;
import javafx.scene.Scene;
import javafx.scene.layout.StackPane;
import javafx.stage.Stage;
/**
  *
  */
public class ApplyCssTest extends Application {
     public void start(Stage primaryStage) {
         System.out.println("One by one: " + addToSceneOneByOne() +
"ms");
         System.out.println("All at once: " + addToSceneAllAtOnce() +
"ms");
         Platform.exit();
     }
     public static void main(String[] args) {
         launch(args);
     }
     public long addToSceneOneByOne() {
         StackPane root = new StackPane();
         Scene scene = new Scene(root, 300, 250);
         long start = System.currentTimeMillis();
         StackPane firstChild = new StackPane();
         root.getChildren().add(firstChild); //add to the scene graph
first
         addNodesOneByOne(1000, firstChild); //then add children one
by one
         return System.currentTimeMillis() - start;
     }
     public long addToSceneAllAtOnce() {
         StackPane root = new StackPane();
         Scene scene = new Scene(root, 300, 250);
         long start = System.currentTimeMillis();
         StackPane firstChild = new StackPane();
         addNodesOneByOne(1000, firstChild); //build the node up, then
         root.getChildren().add(firstChild); //add to scene graph all
at once
         return System.currentTimeMillis() - start;
     }
     public void addNodesOneByOne(int numToAdd, StackPane parent) {
         StackPane last = parent;
         for (int i = 0; i < numToAdd; i++) {
             StackPane curr = new StackPane();
             last.getChildren().add(curr);
             last = curr;
         }
     }
}

[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

Post by David Grieve
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

Post by David Grieve
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.