Re: JDK-8193445 Performance Test (CSS applied redundantly)

[Dean Wookey]

The last time the issue was discussed was in November. You can see the
thread here:
https://mail.openjdk.java.net/pipermail/openjfx-dev/2018-November/022837.html

I too would like to see this and other performance issues addressed
(although our actual biggest priority is mobile). Right now my team is
building javafx ourselves with this fix:
https://github.com/DeanWookey/openjdk-jfx/commit/65a1ed82bce262294f1969e9a12e1126ec8a1ec6
and
we've been quite happy with it. Our application is fairly complex, and our
tables are particularly susceptible to css bugs. Our team hasn't seen
anything weird happen at all since we started using it. That may be a
solution for you until it's addressed. Alternatively you could try adding
the nodes to the scene graph incrementally, possibly even automatically
breaking a complex node into parts and rebuilding it while it's attached to
the scene. Something like this (I haven't actually tested this):

public void addComplex() {
BorderPane borderPane = new BorderPane();
Pane complexNode = new Pane();
addChild((n) -> borderPane.setCenter(n), complexNode);

//or
StackPane root = new StackPane();
Pane complexNode2 = new Pane();
addChild((n) -> root.getChildren().add(n), complexNode2);
}

public static void addChild(Consumer addChild, Node child) {
if (child instanceof BorderPane) {
BorderPane childBorderPane = (BorderPane) child;
Node top = childBorderPane.getTop();
Node bottom = childBorderPane.getBottom();
Node left = childBorderPane.getLeft();
Node right = childBorderPane.getRight();
Node center = childBorderPane.getCenter();
ArrayList all = copy(childBorderPane.getChildren());
childBorderPane.getChildren().clear();
addChild.accept(childBorderPane);
addChild((n) -> childBorderPane.setTop(n), top);
addChild((n) -> childBorderPane.setBottom(n), bottom);
addChild((n) -> childBorderPane.setLeft(n), left);
addChild((n) -> childBorderPane.setRight(n), right);
addChild((n) -> childBorderPane.setCenter(n), center);
for (Node node : all) {
if (!childBorderPane.getChildren().contains(node)) {
addChild((n) -> childBorderPane.getChildren().add(n),
node);
}
}
} else if (child instanceof Pane) {
Pane childPane = (Pane) child;
ArrayList all = copy(childPane.getChildren());
childPane.getChildren().clear();
addChild.accept(child);
for (Node node : all) {
addChild((n) -> childPane.getChildren().add(n), node);
}
} else if (child instanceof Group) {
Group childGroup = (Group) child;
ArrayList all = copy(childGroup.getChildren());
childGroup.getChildren().clear();
addChild.accept(child);
for (Node node : all) {
addChild((n) -> childGroup.getChildren().add(n), node);
}
} else {
addChild.accept(child);
}
}

private static ArrayList copy(ObservableList nodes) {
ArrayList copy = new ArrayList<>();
for (Node n : nodes) {
copy.add(n);
}
return copy;
}

On Thu, Jan 17, 2019 at 3:06 PM  wrote:

> Does anybody has any information about a target release for fixing
> https://bugs.openjdk.java.net/browse/JDK-8183100 , this issue causing
> severe
> performance degradation since JDK1.8.172?
>
>
>
> Since JDK1.8.182, CSS is applied redundantly, leading too significant
> performance degradation, on scenes with many levels of hierarchy.
>
>
>
> I logged this issue in two JIRA in December 2018 (more than 1 year ago) :
>
> https://bugs.openjdk.java.net/browse/JDK-8193445
>
> https://bugs.openjdk.java.net/browse/JDK-8209830
>
>
>
> I've reported another issue
> https://bugs.openjdk.java.net/browse/JDK-8199216
> , showing performance and memory issue caused by Javafx PseudoClassState
> objects. This issue could be related to the previous ones.
>
>
>
> This is a really BLOCKING issue, our product is stuck with JDK1.8.162,
> blocking us for trying a migration to JDK10 or JDK11!
>
>
>
> Daniel Weil (ARGOSIM)
>
>

Re: JDK-8193445 Performance Test (CSS applied redundantly)

[Scott Palmer]

> On Jan 17, 2019, at 3:23 PM, Tom Schindl  wrote:
> 
> Sorry to hi-jack but:
> 
>> Performance improvements and a JNI mechanism to get at native window handles 
>> are the first two items on my wish list.  Though extendable media support 
>> could potentially beat out the window handle thing.. I only need a native 
>> window handle so I can do native rendering of video frames from a non-JavaFX 
>> source.
> 
> Couldn't you leverage our DriftFX work for this?

Yes, possibly!  Thanks for reminding me. I did make note of it last month but 
it fell off the radar over the holidays. 

Scott

Re: JDK-8193445 Performance Test (CSS applied redundantly)

[Tom Schindl]

Sorry to hi-jack but:

> Performance improvements and a JNI mechanism to get at native window handles 
> are the first two items on my wish list.  Though extendable media support 
> could potentially beat out the window handle thing.. I only need a native 
> window handle so I can do native rendering of video frames from a non-JavaFX 
> source.

Couldn't you leverage our DriftFX work for this?

Tom

-- 
Tom Schindl, CTO
BestSolution.at EDV Systemhaus GmbH
Eduard-Bodem-Gasse 5-7. A-6020 Innsbruck
Reg. Nr. FN 222302s am Firmenbuchgericht Innsbruck

Re: JDK-8193445 Performance Test (CSS applied redundantly)

[Scott Palmer]

JDK-8183100 is a fixed issue.  But it basically undid the fix for 
https://bugs.openjdk.java.net/browse/JDK-8151756 

I think you mean to ask when JDK-8193445 might be addressed.

I too would like to see the performance return, and actually I’m hoping it can 
be made better than it was before. 

Performance improvements and a JNI mechanism to get at native window handles 
are the first two items on my wish list.  Though extendable media support could 
potentially beat out the window handle thing.. I only need a native window 
handle so I can do native rendering of video frames from a non-JavaFX source.

Scott 

> On Jan 17, 2019, at 8:05 AM,  
>  wrote:
> 
> Does anybody has any information about a target release for fixing
> https://bugs.openjdk.java.net/browse/JDK-8183100 , this issue causing severe
> performance degradation since JDK1.8.172?
> 
> 
> 
> Since JDK1.8.182, CSS is applied redundantly, leading too significant
> performance degradation, on scenes with many levels of hierarchy.
> 
> 
> 
> I logged this issue in two JIRA in December 2018 (more than 1 year ago) : 
> 
> https://bugs.openjdk.java.net/browse/JDK-8193445
> 
> https://bugs.openjdk.java.net/browse/JDK-8209830
> 
> 
> 
> I've reported another issue https://bugs.openjdk.java.net/browse/JDK-8199216
> , showing performance and memory issue caused by Javafx PseudoClassState
> objects. This issue could be related to the previous ones.
> 
> 
> 
> This is a really BLOCKING issue, our product is stuck with JDK1.8.162,
> blocking us for trying a migration to JDK10 or JDK11!
> 
> 
> 
> Daniel Weil (ARGOSIM)
> 

Re: JDK-8193445 Performance Test (CSS applied redundantly)

[daniel.weil]

Does anybody has any information about a target release for fixing
https://bugs.openjdk.java.net/browse/JDK-8183100 , this issue causing severe
performance degradation since JDK1.8.172?

 

Since JDK1.8.182, CSS is applied redundantly, leading too significant
performance degradation, on scenes with many levels of hierarchy.

 

I logged this issue in two JIRA in December 2018 (more than 1 year ago) : 

https://bugs.openjdk.java.net/browse/JDK-8193445

https://bugs.openjdk.java.net/browse/JDK-8209830

 

I've reported another issue https://bugs.openjdk.java.net/browse/JDK-8199216
, showing performance and memory issue caused by Javafx PseudoClassState
objects. This issue could be related to the previous ones.

 

This is a really BLOCKING issue, our product is stuck with JDK1.8.162,
blocking us for trying a migration to JDK10 or JDK11!

 

Daniel Weil (ARGOSIM)

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

Re: JDK-8193445 Performance Test

[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


On 11/7/2018 5:37 AM, David Grieve 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.



On 11/7/18 2:57 AM, Dean Wookey wrote:

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;
 }
 }
}

Re: JDK-8193445 Performance Test

[Scott Palmer]

> On Nov 7, 2018, at 8:37 AM, David Grieve  wrote:
> 
> ...
> 
> 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

Re: JDK-8193445 Performance Test

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



On 11/7/18 2:57 AM, Dean Wookey wrote:

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;
 }
 }
}

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;
}
}
}