Re: RFR: 8252387: Deprecate for removal css Selector and ShapeConverter constructors [v2]
On Sat, 29 Aug 2020 14:53:37 GMT, Kevin Rushforth wrote: >> @kevinrushforth Why is the Skara bot saying this PR can be integrated if the >> CSR is not approved yet? >> >> Also, in the main post under Reviewers, the links to the users lead to a 404. >> >> Should I submit bugs for these? > >> Why is the Skara bot saying this PR can be integrated if the CSR is not >> approved yet? > > Looks like we both noticed this at the same time. > >> Also, in the main post under Reviewers, the links to the users lead to a 404. > > I hadn't noticed this before. > >> Should I submit bugs for these? > > Yes, please. @bhaweshkc the CSR is approved. Go ahead and `/integrate` when ready. - PR: https://git.openjdk.java.net/jfx/pull/290
Re: Possible approaches to JDK-8185886: Improve scrolling performance of TableView and TreeTableView
Hi Jeanette,
Thanks for taking a look.
The PR was sort of inspired by JDK-8185886 (PR 108) because changes were
made there to make performance with long listener lists better (by using
a Map structure). I wanted to know the root cause of the long lists,
and found out that the long lists are on properties carried by Window
and Scene. This then was traced to the treeShowing property in Node
which registers listeners on Window and Scene.
I think Kevin then mentioned the JDK-8090322 issue which gave me further
background on why the treeShowing solution was introduced.
The change I proposed to fix this regression was also tested by the
author of PR 108 (Danny Gonzalez) and addressed the issue as well, see
this comment: https://github.com/openjdk/jfx/pull/108#issuecomment-615125904
I'm not aware of any other side effects of the change introduces with
JDK-8090322, but anything with lots of nodes in a single scene (in the
thousands) may see some performance degradation when adding/removing
nodes -- complex Tables or TreeTables are the primary culprit. It might
also affect the performance of hiding/showing a Window (when there are
many nodes), but that's not an action that is usually done a lot.
I did not include a JDK number -- I can include JDK-8185886 as it was
supposed to address that issue, or file a new issue referring both that
issue and JDK-8090322.
I donot think it will fix the performance issue with having many columns
in a table, although it may reduce its impact a bit. The issue there is
more that columns are not virtualized, so a table rotated 90 degrees
would exhibit the same problems as an unvirtualized table would because
of the sheer number of nodes involved.
--John
On 02/09/2020 14:24, Jeanette Winzenburg wrote:
Hi John,
thanks for the clarification :)
Hmm .. but then it's not really a PR against JDK-8185886 (scrolling
performance was always bad with many columns) but against - yet to be
reported - side-effect of JDK-8090322 which happens to detoriate
tableView performance even further (there might be other side-effects)?
-- Jeanette
Zitat von John Hendrikx :
The "dynamic update performance" performance issue we are seeing is a
regression from JDK-8090322. In this change the Node treeShowing
property was introduced. The JDK-8090322 warns in its description about:
"""Considerations:
* This is too expensive to calculate for all nodes by default. So the
simplest way to provide it would be a special binding implementation
or a util class. Where you create a instance and pass in the node you
are interested in. It can then register listeners all the way up the
tree and listen to what it needs. """
The above comment is warning against the fact that registering
listeners for EACH Node on Window and Scene is going to be a
performance issue. As nodes can number in the 1000's or 10.000's,
that's a lot of listeners to store in a List data structure.
PR 185 targets this issue and implements the feature in JDK-8090322 in
the way that was suggested in the above comment, instead of how it
currently is implemented (registering listeners for every Node, just
in case someone needs the treeShowing property).
It's scope is not as broad as it would seem (because of a change in
Node). It effectively only makes a small change in two controls
(PopupWindow and ProgressIndicatorSkin).
--John
On 31/08/2020 16:54, Jeanette Winzenburg wrote:
Looking at the examples provided in 108/125: apart from both having many
columns (> 300 makes them really nasty) they differ in
Table content:
125 - static data
108 - items are frequently modified (added)
Perceived performance:
125 - vertical scrolling: thumb/content lags behind mouse
108 - with enough columns, all interaction is sluggish (mouse, keys,
..), scrolling being just one of them
Both have examples, running those against the suggested fixes (with 108a
for Jose's approach)
125 - fixes its own example, does nothing for the other
108, 108a, 185 - improves its own example, does nothing for other
So we seem to have multiple issues that are (mostly) orthogonal: one
being the broken/missing horizontal virtualization (125), the other
related to dynamic update of table content (108, 108a, 185). We need to
solve both, the solution/s for one looks (mostly?) unrelated to the
solution to the other.
125 is the only one PR for its use-case, and it seems to do its job.
From those targeting the dynamic data update all except Jose's (not yet
formalized into a PR) approach feel too broad: table's reaction to items
modifications is .. suboptimal in more than one aspect. Changing overall
notification implementation to improve that, sounds like covering it up.
-- Jeanette
Zitat von Kevin Rushforth :
Sorry for the badly formatted html. Here it is again.
I see some progress being made on the {Tree}TableView performance
issue. To summarize where I think we are:
There are currently 2 different approaches under review:
1.
Re: RFR: 8217472: Add attenuation for PointLight [v5]
On Wed, 2 Sep 2020 16:57:25 GMT, Nir Lisker wrote:
>> It can either be a fully qualified class name (with `.` as separator) or the
>> unqualified name of the test class. The
>> class name must end with exactly `Test`. So for example, try it with
>> `Snapshot1Test`.
>
> I wrote the following test:
>
> import static org.junit.Assert.assertTrue;
> import static org.junit.Assert.fail;
>
> import java.util.concurrent.CountDownLatch;
> import java.util.concurrent.TimeUnit;
>
> import org.junit.AfterClass;
> import org.junit.BeforeClass;
> import org.junit.Test;
>
> import javafx.application.Application;
> import javafx.application.Platform;
> import javafx.scene.Group;
> import javafx.scene.PointLight;
> import javafx.scene.Scene;
> import javafx.scene.paint.Color;
> import javafx.scene.shape.Box;
> import javafx.stage.Stage;
> import javafx.stage.WindowEvent;
>
> public class PointLightAttenuationTest {
>
> private static CountDownLatch startupLatch;
> private static Stage stage;
> private static PointLight light = new PointLight(Color.BLUE);
> private static Box box = new Box(100, 100, 1);
>
> @BeforeClass
> public static void initFX() throws Exception {
> startupLatch = new CountDownLatch(1);
> new Thread(() -> Application.launch(TestApp.class,
> (String[])null)).start();
> assertTrue("Timeout waiting for FX runtime to start",
> startupLatch.await(15, TimeUnit.SECONDS));
> }
>
> public class TestApp extends Application {
>
> @Override
> public void start(Stage mainStage) {
> stage = mainStage;
> light.setTranslateZ(-50);
> var root = new Group(light, box);
> var scene = new Scene(root);
> stage.setScene(scene);
> stage.setFullScreen(true);
> stage.addEventHandler(WindowEvent.WINDOW_SHOWN, e ->
> Platform.runLater(startupLatch::countDown));
> stage.show();
> }
> }
>
> @Test
> public void testAttenuation() {
> var image = box.snapshot(null, null);
> var nonAttenColor = image.getPixelReader().getColor(1, 1);
>
> light.setLinearAttenuation(2);
> image = box.snapshot(null, null);
> var attenColor = image.getPixelReader().getColor(1, 1);
>
> System.out.println(nonAttenColor);
> System.out.println(attenColor);
> if (nonAttenColor.getBlue() > attenColor.getBlue()) {
> throw new AssertionError("Attenuation color should be less than
> non-attenuated");
> }
> }
>
> @AfterClass
> public static void teardown() {
> Platform.runLater(() -> {
> stage.hide();
> Platform.exit();
> });
> }
> }
> But when executing it with
>
> ./gradlew -PFULL_TEST=true -PUSE_ROBOT=true :systemTests:test --tests
> PointLightAttenuationTest
>
> I get the error
>
> test.javafx.scene.lighting3D.PointLightAttenuationTest > classMethod FAILED
> java.lang.AssertionError: Timeout waiting for FX runtime to start
> at org.junit.Assert.fail(Assert.java:91)
> at org.junit.Assert.assertTrue(Assert.java:43)
> at
> test.javafx.scene.lighting3D.PointLightAttenuationTest.initFX(PointLightAttenuationTest.java:59)
>
> So for some reason the Application doesn't start, it seems. I ran
> `ShapeViewOrderLeakTest` and `RestoreSceneSizeTest`
> which look the same, and those work. Any idea?
If you run it with `--info` you will see something like this:
test.javafx.scene.shape.PointLightAttenuationTest STANDARD_ERROR
Exception in Application constructor
Exception in thread "Thread-4" java.lang.RuntimeException: Unable to
construct Application instance: class
test.javafx.scene.shape.PointLightAttenuationTest$TestApp
at
javafx.graphics/com.sun.javafx.application.LauncherImpl.launchApplication1(LauncherImpl.java:890)
at
javafx.graphics/com.sun.javafx.application.LauncherImpl.lambda$launchApplication$2(LauncherImpl.java:195)
at java.base/java.lang.Thread.run(Thread.java:832)
Caused by: java.lang.NoSuchMethodException:
test.javafx.scene.shape.PointLightAttenuationTest$TestApp.()
at java.base/java.lang.Class.getConstructor0(Class.java:3427)
at java.base/java.lang.Class.getConstructor(Class.java:2165)
at
javafx.graphics/com.sun.javafx.application.LauncherImpl.lambda$launchApplication1$8(LauncherImpl.java:801)
at
javafx.graphics/com.sun.javafx.application.PlatformImpl.lambda$runAndWait$12(PlatformImpl.java:455)
at
javafx.graphics/com.sun.javafx.application.PlatformImpl.lambda$runLater$10(PlatformImpl.java:428)
at
java.base/java.security.AccessController.doPrivileged(AccessController.java:391)
at
javafx.graphics/com.sun.javafx.application.PlatformImpl.lambda$runLater$11(PlatformImpl.java:427)
at
Re: RFR: 8217472: Add attenuation for PointLight [v5]
On Tue, 1 Sep 2020 23:09:39 GMT, Kevin Rushforth wrote:
>>> gradle -PFULL_TEST=true -PUSE_ROBOT=true :systemTests:test --tests MyTest
>>
>> What format is `MyTest`? Is it some relative path?
>
> It can either be a fully qualified class name (with `.` as separator) or the
> unqualified name of the test class. The
> class name must end with exactly `Test`. So for example, try it with
> `Snapshot1Test`.
I wrote the following test:
import static org.junit.Assert.assertTrue;
import static org.junit.Assert.fail;
import java.util.concurrent.CountDownLatch;
import java.util.concurrent.TimeUnit;
import org.junit.AfterClass;
import org.junit.BeforeClass;
import org.junit.Test;
import javafx.application.Application;
import javafx.application.Platform;
import javafx.scene.Group;
import javafx.scene.PointLight;
import javafx.scene.Scene;
import javafx.scene.paint.Color;
import javafx.scene.shape.Box;
import javafx.stage.Stage;
import javafx.stage.WindowEvent;
public class PointLightAttenuationTest {
private static CountDownLatch startupLatch;
private static Stage stage;
private static PointLight light = new PointLight(Color.BLUE);
private static Box box = new Box(100, 100, 1);
@BeforeClass
public static void initFX() throws Exception {
startupLatch = new CountDownLatch(1);
new Thread(() -> Application.launch(TestApp.class,
(String[])null)).start();
assertTrue("Timeout waiting for FX runtime to start",
startupLatch.await(15, TimeUnit.SECONDS));
}
public class TestApp extends Application {
@Override
public void start(Stage mainStage) {
stage = mainStage;
light.setTranslateZ(-50);
var root = new Group(light, box);
var scene = new Scene(root);
stage.setScene(scene);
stage.setFullScreen(true);
stage.addEventHandler(WindowEvent.WINDOW_SHOWN, e ->
Platform.runLater(startupLatch::countDown));
stage.show();
}
}
@Test
public void testAttenuation() {
var image = box.snapshot(null, null);
var nonAttenColor = image.getPixelReader().getColor(1, 1);
light.setLinearAttenuation(2);
image = box.snapshot(null, null);
var attenColor = image.getPixelReader().getColor(1, 1);
System.out.println(nonAttenColor);
System.out.println(attenColor);
if (nonAttenColor.getBlue() > attenColor.getBlue()) {
throw new AssertionError("Attenuation color should be less than
non-attenuated");
}
}
@AfterClass
public static void teardown() {
Platform.runLater(() -> {
stage.hide();
Platform.exit();
});
}
}
But when executing it with
./gradlew -PFULL_TEST=true -PUSE_ROBOT=true :systemTests:test --tests
PointLightAttenuationTest
I get the error
test.javafx.scene.lighting3D.PointLightAttenuationTest > classMethod FAILED
java.lang.AssertionError: Timeout waiting for FX runtime to start
at org.junit.Assert.fail(Assert.java:91)
at org.junit.Assert.assertTrue(Assert.java:43)
at
test.javafx.scene.lighting3D.PointLightAttenuationTest.initFX(PointLightAttenuationTest.java:59)
So for some reason the Application doesn't start, it seems. I ran
`ShapeViewOrderLeakTest` and `RestoreSceneSizeTest`
which look the same, and those work. Any idea?
-
PR: https://git.openjdk.java.net/jfx/pull/43
Integrated: 8252062: WebKit build fails with recent VS 2019 compiler
On Wed, 2 Sep 2020 11:22:45 GMT, Arun Joseph wrote: > The WebKit build fails with recent VS 2019 compiler. > > Bug: This MediaQueryEvaluator constructor takes a String parameter, but the > conversion of char* to String is failing. > > Fix: Use the default constructor of MediaQueryEvaluator, which also returns > true for "all". > > Test: Build webkit with the recent VS 2019 compiler with and without this > fix. It should fail without the fix and build > with the fix. This pull request has now been integrated. Changeset: 8e3616cf Author:Arun Joseph URL: https://git.openjdk.java.net/jfx/commit/8e3616cf Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod 8252062: WebKit build fails with recent VS 2019 compiler Reviewed-by: kcr - PR: https://git.openjdk.java.net/jfx/pull/294
Re: RFR: 8252062: WebKit build fails with recent VS 2019 compiler
On Wed, 2 Sep 2020 15:33:23 GMT, Kevin Rushforth wrote: >> The WebKit build fails with recent VS 2019 compiler. >> >> Bug: This MediaQueryEvaluator constructor takes a String parameter, but the >> conversion of char* to String is failing. >> >> Fix: Use the default constructor of MediaQueryEvaluator, which also returns >> true for "all". >> >> Test: Build webkit with the recent VS 2019 compiler with and without this >> fix. It should fail without the fix and build >> with the fix. > > Marked as reviewed by kcr (Lead). This is simple enough that a single reviewer will suffice. - PR: https://git.openjdk.java.net/jfx/pull/294
Re: RFR: 8252062: WebKit build fails with recent VS 2019 compiler
On Wed, 2 Sep 2020 11:22:45 GMT, Arun Joseph wrote: > The WebKit build fails with recent VS 2019 compiler. > > Bug: This MediaQueryEvaluator constructor takes a String parameter, but the > conversion of char* to String is failing. > > Fix: Use the default constructor of MediaQueryEvaluator, which also returns > true for "all". > > Test: Build webkit with the recent VS 2019 compiler with and without this > fix. It should fail without the fix and build > with the fix. Marked as reviewed by kcr (Lead). - PR: https://git.openjdk.java.net/jfx/pull/294
Re: Possible approaches to JDK-8185886: Improve scrolling performance of TableView and TreeTableView
Hi John,
thanks for the clarification :)
Hmm .. but then it's not really a PR against JDK-8185886 (scrolling
performance was always bad with many columns) but against - yet to be
reported - side-effect of JDK-8090322 which happens to detoriate
tableView performance even further (there might be other side-effects)?
-- Jeanette
Zitat von John Hendrikx :
The "dynamic update performance" performance issue we are seeing is
a regression from JDK-8090322. In this change the Node treeShowing
property was introduced. The JDK-8090322 warns in its description
about:
"""Considerations:
* This is too expensive to calculate for all nodes by default. So
the simplest way to provide it would be a special binding
implementation or a util class. Where you create a instance and pass
in the node you are interested in. It can then register listeners
all the way up the tree and listen to what it needs. """
The above comment is warning against the fact that registering
listeners for EACH Node on Window and Scene is going to be a
performance issue. As nodes can number in the 1000's or 10.000's,
that's a lot of listeners to store in a List data structure.
PR 185 targets this issue and implements the feature in JDK-8090322 in
the way that was suggested in the above comment, instead of how it
currently is implemented (registering listeners for every Node, just
in case someone needs the treeShowing property).
It's scope is not as broad as it would seem (because of a change in
Node). It effectively only makes a small change in two controls
(PopupWindow and ProgressIndicatorSkin).
--John
On 31/08/2020 16:54, Jeanette Winzenburg wrote:
Looking at the examples provided in 108/125: apart from both having many
columns (> 300 makes them really nasty) they differ in
Table content:
125 - static data
108 - items are frequently modified (added)
Perceived performance:
125 - vertical scrolling: thumb/content lags behind mouse
108 - with enough columns, all interaction is sluggish (mouse, keys,
..), scrolling being just one of them
Both have examples, running those against the suggested fixes (with 108a
for Jose's approach)
125 - fixes its own example, does nothing for the other
108, 108a, 185 - improves its own example, does nothing for other
So we seem to have multiple issues that are (mostly) orthogonal: one
being the broken/missing horizontal virtualization (125), the other
related to dynamic update of table content (108, 108a, 185). We need to
solve both, the solution/s for one looks (mostly?) unrelated to the
solution to the other.
125 is the only one PR for its use-case, and it seems to do its job.
From those targeting the dynamic data update all except Jose's (not yet
formalized into a PR) approach feel too broad: table's reaction to items
modifications is .. suboptimal in more than one aspect. Changing overall
notification implementation to improve that, sounds like covering it up.
-- Jeanette
Zitat von Kevin Rushforth :
Sorry for the badly formatted html. Here it is again.
I see some progress being made on the {Tree}TableView performance
issue. To summarize where I think we are:
There are currently 2 different approaches under review:
1. https://github.com/openjdk/jfx/pull/108 : optimization in
javafx.base to make removing listeners faster
2. https://github.com/openjdk/jfx/pull/125 : optimization in TableView
to reduce the number of add / removes
In addition, the following is a WIP PR that the author thinks could be
a 3rd approach:
3. https://github.com/openjdk/jfx/pull/185 : optimization in scene
graph to avoid install treeShowing listeners on Window and Scene for
all nodes
Jose has proposed a 4th approach as a comment to PR #108, and as I
understand it, he will propose a PR shortly.
4. Don't clear the list of children in a VirtualFlow when the number
of items changes.
So the first thing that is needed is to evaluate the approaches and
decide which one to pursue.
Options 1 and 3 are more broad in their scope, option #2 is more
targeted (to TableView), but within that area is a larger change.
Option #3 would remove the (internal) treeShowing property as a
generally available concept and only use it for controls like
ProgressIndicator that really need it. Option #4 affects only controls
that use VirtualFlow (ListView, TableVIew, TreeTableView), and seems
not to be a large change (presuming we can verify that no leak is
introduced).
I note that these fixes are not mutually exclusive, but I do think we
need to settle on a primary approach and use that to fix this issue.
If there are still performance problems after that fix, we can
consider one (or more) of the others.
Comments?
-- Kevin
RFR: 8252062: WebKit build fails with recent VS 2019 compiler
The WebKit build fails with recent VS 2019 compiler. Bug: This MediaQueryEvaluator constructor takes a String parameter, but the conversion of char* to String is failing. Fix: Use the default constructor of MediaQueryEvaluator, which also returns true for "all". Test: Build webkit with the recent VS 2019 compiler with and without this fix. It should crash without the fix and build with the fix. - Commit messages: - 8252062: WebKit build fails with recent VS 2019 compiler Changes: https://git.openjdk.java.net/jfx/pull/294/files Webrev: https://webrevs.openjdk.java.net/?repo=jfx=294=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8252062 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jfx/pull/294.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/294/head:pull/294 PR: https://git.openjdk.java.net/jfx/pull/294
Re: RFR: 8185886: Improve scrolling performance of TableView and TreeTableView [v3]
On Tue, 1 Sep 2020 20:57:58 GMT, yosbits wrote: > > > When the startup time is measured by eye, the impression changes depending on > the individual difference. my eye is a precision instrument :) Seriously, who would do such a thingy? Obviously, it must be measured, for zeroth approximation doing so crudely by comparing currentMillis before/after showing is good enough to "see" the tendency. > The effect of runLater affects your experience. that's why you shouldn't do it when trying to gain insight into timing issues ;) > > However, I succeeded in further improving performance by eliminating another > bottleneck in TreeTableView. Of course, it > also includes improvements in startup time. cool :) - PR: https://git.openjdk.java.net/jfx/pull/125
