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 1 commit
Commits
File filter...
Filter file types
Jump to…
Jump to file or symbol
Failed to load files and symbols.

Always

Just for now

Fix for functional regressions of JDK-8151756 + add a sytem test

  • Loading branch information
aghaisas committed Nov 12, 2019
commit 12ea8220a730ff8d98d520ce870691d54f0de00f
@@ -1184,8 +1184,10 @@ public final Scene getScene() {
* Exists for Parent and LightBase
*/
void scenesChanged(final Scene newScene, final SubScene newSubScene,
final Scene oldScene, final SubScene oldSubScene) { }

final Scene oldScene, final SubScene oldSubScene) {
// On scenes change, reapply CSS for this Node
reapplyCSS();
}

/**
* The id of this {@code Node}. This simple string identifier is useful for
@@ -756,6 +756,7 @@ 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
@@ -0,0 +1,96 @@
/*
* Copyright (c) 2019, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
* under the terms of the GNU General Public License version 2 only, as
* published by the Free Software Foundation. Oracle designates this
* particular file as subject to the "Classpath" exception as provided
* by Oracle in the LICENSE file that accompanied this code.
*
* This code is distributed in the hope that it will be useful, but WITHOUT
* ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
* FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
* version 2 for more details (a copy is included in the LICENSE file that
* accompanied this code).
*
* You should have received a copy of the GNU General Public License version
* 2 along with this work; if not, write to the Free Software Foundation,
* Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
*
* Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
* or visit www.oracle.com if you need additional information or have any
* questions.
*/

package test.robot.javafx.scene;
This conversation was marked as resolved by aghaisas

This comment has been minimized.

Copy link
@kevinrushforth

kevinrushforth Nov 14, 2019

Collaborator

There is no need for this test to require robot. I recommend moving it to test.javafx.scene (and not inherit from VisualTestBase).


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 org.junit.Test;
import test.robot.testharness.VisualTestBase;
import static org.junit.Assert.assertTrue;

/**
* This test is based on the test case reported in JDK-8209830
*
* Redundant CSS Re-application was avoided in JDK-8193445.
* It results in faster application of CSS on Controls (Nodes). In turn,
* 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
* 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 {
This conversation was marked as resolved by aghaisas

This comment has been minimized.

Copy link
@kevinrushforth

kevinrushforth Nov 14, 2019

Collaborator

We have moved away from putting the bug ID in the test class name, so I recommend renaming it.


private Stage testStage;
private Scene testScene;
private BorderPane pane = new BorderPane();
private long mSec = 0;

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

waitFirstFrame();

// Measure time to create and add 300 Nodes to Scene
runAndWait(() -> {
long startTime = System.currentTimeMillis();

HBox hbox = new HBox();
for (int i = 0; i < 300; i++) {
This conversation was marked as resolved by aghaisas

This comment has been minimized.

Copy link
@kevinrushforth

kevinrushforth Nov 14, 2019

Collaborator

In my testing on various machines, the bug is more pronounced, and less prone to system differences with 500 nodes instead of 300.

hbox = new HBox(new Text("y"), hbox);
final HBox h = hbox;
h.setPadding(new Insets(1));
}
pane.setCenter(hbox);

long endTime = System.currentTimeMillis();

mSec = endTime - startTime;
});

System.out.println("Time to create and add 300 nodes to a Scene = " + mSec + " mSec");

// 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);
This conversation was marked as resolved by aghaisas

This comment has been minimized.

Copy link
@kevinrushforth

kevinrushforth Nov 14, 2019

Collaborator

If you increase the number of nodes to 500 then I recommend bumping the time threshold to 800 msec in case it is run on a very slow system.

}
}
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.