Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8193445: JavaFX CSS is applied redundantly leading to significant performance degradation #34

Closed
Closed
Changes from 3 commits
Commits
File filter...
Filter file types
Jump to…
Jump to file or symbol
Failed to load files and symbols.

Always

Just for now

@@ -1051,15 +1051,13 @@ public void fireSuperValueChangedEvent() {
}
}

// reapplyCSS should be true for root elements when they are added, and is false for children
// of the root element. This prevents CSS being reapplied recursively, as noted in JDK-8151756.
private void invalidatedScenes(Scene oldScene, SubScene oldSubScene, boolean reapplyCSS) {
private void invalidatedScenes(Scene oldScene, SubScene oldSubScene) {
Scene newScene = sceneProperty().get();
boolean sceneChanged = oldScene != newScene;
SubScene newSubScene = subScene;

if (getClip() != null) {
getClip().setScenes(newScene, newSubScene, reapplyCSS);
getClip().setScenes(newScene, newSubScene);
}
if (sceneChanged) {
updateCanReceiveFocus();
@@ -1093,9 +1091,7 @@ private void invalidatedScenes(Scene oldScene, SubScene oldSubScene, boolean rea
}
updateTreeShowing();

if (sceneChanged && reapplyCSS) {
reapplyCSS();
}
if (sceneChanged) reapplyCSS();

if (sceneChanged && !isDirtyEmpty()) {
//Note: no need to remove from scene's dirty list
@@ -1154,16 +1150,16 @@ private void invalidatedScenes(Scene oldScene, SubScene oldSubScene, boolean rea
}
}

final void setScenes(Scene newScene, SubScene newSubScene, boolean reapplyCSS) {
final void setScenes(Scene newScene, SubScene newSubScene) {
Scene oldScene = sceneProperty().get();
if (newScene != oldScene || newSubScene != subScene) {
scene.set(newScene);
SubScene oldSubScene = subScene;
subScene = newSubScene;
invalidatedScenes(oldScene, oldSubScene, reapplyCSS);
invalidatedScenes(oldScene, oldSubScene);
if (this instanceof SubScene) { // TODO: find better solution
SubScene thisSubScene = (SubScene)this;
thisSubScene.getRoot().setScenes(newScene, thisSubScene, reapplyCSS);
thisSubScene.getRoot().setScenes(newScene, thisSubScene);
}
}
}
@@ -1184,10 +1180,8 @@ public final Scene getScene() {
* Exists for Parent and LightBase
*/
void scenesChanged(final Scene newScene, final SubScene newSubScene,
final Scene oldScene, final SubScene oldSubScene) {
// On scenes change, reapply CSS for this Node
reapplyCSS();
}
final Scene oldScene, final SubScene oldSubScene) { }


/**
* The id of this {@code Node}. This simple string identifier is useful for
@@ -6982,13 +6976,13 @@ protected void invalidated() {
} else {
if (oldClip != null) {
oldClip.clipParent = null;
oldClip.setScenes(null, null, /* reapplyCSS */ false);
oldClip.setScenes(null, null);
oldClip.updateTreeVisible(false);
}

if (newClip != null) {
newClip.clipParent = Node.this;
newClip.setScenes(getScene(), getSubScene(), /* reapplyCSS */ false);
newClip.setScenes(getScene(), getSubScene());
newClip.updateTreeVisible(true);
}

@@ -9421,6 +9415,13 @@ final void reapplyCSS() {

if (cssFlag == CssFlags.REAPPLY) return;

if (cssFlag == CssFlags.DIRTY_BRANCH) {
// JDK-8193445 - don't reapply CSS from here
// Defer CSS application to this Node by marking cssFlag as REAPPLY
cssFlag = CssFlags.REAPPLY;
return;
}

// RT-36838 - don't reapply CSS in the middle of an update
if (cssFlag == CssFlags.UPDATE) {
cssFlag = CssFlags.REAPPLY;
@@ -368,7 +368,7 @@ protected void onChanged(Change<Node> c) {
relayout = true;
}
node.setParent(Parent.this);
node.setScenes(getScene(), getSubScene(), /* reapplyCSS */ true);
node.setScenes(getScene(), getSubScene());
// assert !node.boundsChanged;
if (node.isVisible()) {
geomChanged = true;
@@ -601,7 +601,7 @@ protected void onProposedChange(final List<Node> newNodes, int[] toBeRemoved) {
}
if (old.getParent() == Parent.this) {
old.setParent(null);
old.setScenes(null, null, /* reapplyCSS */ false);
old.setScenes(null, null);
}
// Do not add node with null scene to the removed list.
// It will not be processed in the list and its memory
@@ -756,7 +756,6 @@ final void toBack(Node node) {
@Override
void scenesChanged(final Scene newScene, final SubScene newSubScene,
final Scene oldScene, final SubScene oldSubScene) {
super.scenesChanged(newScene, newSubScene, oldScene, oldSubScene);

if (oldScene != null && newScene == null) {
// RT-34863 - clean up CSS cache when Parent is removed from scene-graph
@@ -769,7 +768,7 @@ void scenesChanged(final Scene newScene, final SubScene newSubScene,
}

for (int i=0; i<children.size(); i++) {
children.get(i).setScenes(newScene, newSubScene, /* reapplyCSS */ false);
children.get(i).setScenes(newScene, newSubScene);
}

final boolean awaitingLayout = layoutFlag != LayoutFlags.CLEAN;
@@ -788,7 +787,6 @@ void scenesChanged(final Scene newScene, final SubScene newSubScene,
}
}
}

}

@Override
@@ -1218,11 +1218,11 @@ protected void invalidated() {
}

if (oldRoot != null) {
oldRoot.setScenes(null, null, /* reapplyCSS */ false);
oldRoot.setScenes(null, null);
}
oldRoot = _value;
_value.getStyleClass().add(0, "root");
_value.setScenes(Scene.this, null, /* reapplyCSS */ true);
_value.setScenes(Scene.this, null);
markDirty(DirtyBits.ROOT_DIRTY);
_value.resize(getWidth(), getHeight()); // maybe no-op if root is not resizable
_value.requestLayout();
@@ -313,11 +313,11 @@ protected void invalidated() {

if (oldRoot != null) {
StyleManager.getInstance().forget(SubScene.this);
oldRoot.setScenes(null, null, /* reapplyCSS */ false);
oldRoot.setScenes(null, null);
}
oldRoot = _value;
_value.getStyleClass().add(0, "root");
_value.setScenes(getScene(), SubScene.this, /* reapplyCSS */ true);
_value.setScenes(getScene(), SubScene.this);
markDirty(SubSceneDirtyBits.ROOT_SG_DIRTY);
_value.resize(getWidth(), getHeight()); // maybe no-op if root is not resizable
_value.requestLayout();
@@ -23,17 +23,26 @@
* questions.
*/

package test.robot.javafx.scene;
package test.javafx.scene;

import javafx.application.Application;
import javafx.application.Platform;
import javafx.geometry.Insets;
import javafx.scene.Scene;
import javafx.scene.layout.HBox;
import javafx.scene.layout.BorderPane;
import javafx.scene.text.Text;
import javafx.stage.Stage;
import javafx.stage.WindowEvent;

import java.util.concurrent.CountDownLatch;
import java.util.concurrent.TimeUnit;

import test.util.Util;
import junit.framework.Assert;
import org.junit.Test;
import test.robot.testharness.VisualTestBase;
import org.junit.AfterClass;
import org.junit.BeforeClass;
import static org.junit.Assert.assertTrue;

/**
@@ -44,53 +53,73 @@
* resulting in improved Node creation/addition time to a Scene.
*
* The goal of this test is *NOT* to measure absolute performance, but to show
* creating and adding 300 Nodes to a scene does not take more than a
* creating and adding 500 Nodes to a scene does not take more than a
* particular threshold of time.
*
* The selected thresold is larger than actual observed time.
* It is not a benchmark value. It is good enough to catch the regression
* in performance, if any.
*/

public class CSSPerf_JDK8193445Test extends VisualTestBase {
public class QuadraticCssTimeTest {

private Stage testStage;
private Scene testScene;
private BorderPane pane = new BorderPane();
private long mSec = 0;
private static CountDownLatch startupLatch;
private static Stage stage;
private static BorderPane rootPane;

@Test(timeout = 15000)
public void testTimeForAdding300NodesToScene() {
runAndWait(() -> {
testStage = getStage();
testScene = new Scene(pane);
testStage.setScene(testScene);
testStage.show();
});
public static class TestApp extends Application {

@Override
public void start(Stage primaryStage) throws Exception {
stage = primaryStage;
rootPane = new BorderPane();
stage.setScene(new Scene(rootPane));
stage.addEventHandler(WindowEvent.WINDOW_SHOWN, e -> {
Platform.runLater(() -> startupLatch.countDown());
});
stage.show();
}
}

@BeforeClass
public static void initFX() throws Exception {
startupLatch = new CountDownLatch(1);
new Thread(() -> Application.launch(QuadraticCssTimeTest.TestApp.class, (String[]) null)).start();

waitFirstFrame();
assertTrue("Timeout waiting for FX runtime to start", startupLatch.await(15, TimeUnit.SECONDS));
}

// Measure time to create and add 300 Nodes to Scene
runAndWait(() -> {
@Test
public void testTimeForAdding500NodesToScene() throws Exception {

This conversation was marked as resolved by aghaisas

This comment has been minimized.

Copy link
@kevinrushforth

kevinrushforth Nov 25, 2019

Collaborator

Adding a node to a live scene graph must be done on the JavaFX Application thread. I recommend wrapping the body of this method in a Util.runAndWait call.

Util.runAndWait(() -> {
// Compute time for adding 500 Nodes
long startTime = System.currentTimeMillis();

HBox hbox = new HBox();
for (int i = 0; i < 300; i++) {
for (int i = 0; i < 500; i++) {
hbox = new HBox(new Text("y"), hbox);
final HBox h = hbox;
h.setPadding(new Insets(1));
}
pane.setCenter(hbox);
rootPane.setCenter(hbox);

long endTime = System.currentTimeMillis();

mSec = endTime - startTime;
});
System.out.println("Time to create and add 500 nodes to a Scene = " +
(endTime - startTime) + " mSec");

System.out.println("Time to create and add 300 nodes to a Scene = " + mSec + " mSec");
// NOTE : 800 mSec is not a benchmark value
// It is good enough to catch the regression in performance, if any
assertTrue("Time to add 500 Nodes is more than 800 mSec", (endTime - startTime) < 800);
});
}

// NOTE : 400 mSec is not a benchmark value
// It is good enough to catch the regression in performance, if any
assertTrue("Time to add 300 Nodes is more than 400 mSec", mSec < 400);
@AfterClass
public static void teardownOnce() {
Platform.runLater(() -> {
stage.hide();
Platform.exit();
});
}
}
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.