[11] JDK-8204621: Upgrade MarlinFX to 0.9.2
Laurent Bourgès
bourges.laurent at gmail.com
Tue Jul 3 09:13:30 UTC 2018
Kevin,
Thanks for the review !
2018-06-29 22:52 GMT+02:00 Kevin Rushforth <kevin.rushforth at oracle.com>:
> I'm giving a +1 on the implementation changes. I scanned the webrev and
> didn't see anything out of place. I compared the diffs of the FX Marlin
> 0.9.2 with the Java2D 0.9.1 changeset, and there were a few more diffs than
> I might have expoected, but nothing jumped out of me as a problem. Also,
> I've tested it pretty well on all three platforms.
>
> The overall +1 is pending the fixes needed for the test: at least the
> copyright header and shortening or disabling the test.
>
Here is the updated webrev fixing the test:
http://cr.openjdk.java.net/~lbourges/marlinFX/marlinFX-092.1/
ClipShapeTest incremental diff:
--- /tmp/meld-tmpyWO0FS
+++
/home/bourgesl/libs/marlin/branches/marlin-fx-openjdk/src/test/java/test/manual/marlin/ClipShapeTest.java
@@ -4,7 +4,9 @@
*
* 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.
+ * 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
@@ -219,12 +221,10 @@
// cubic min/max error:
System.setProperty("prism.marlin.cubic_dec_d2", "1e-3");
- System.setProperty("prism.marlin.cubic_inc_d1", "1e-4"); // or
disabled ~ 1e-6
+ System.setProperty("prism.marlin.cubic_inc_d1", "1e-4");
// quad max error:
System.setProperty("prism.marlin.quad_dec_d2", "5e-4");
-
- System.setProperty("javafx.animation.fullspeed", "true"); // full
speed
}
// Application class. An instance is created and initialized before
running
@@ -273,7 +273,7 @@
}
private static void resetOptions() {
- NUM_TESTS = 5000;
+ NUM_TESTS = Integer.getInteger("ClipShapeTest.numTests", 100);
// shape settings:
SHAPE_MODE = ShapeMode.NINE_LINE_POLYS;
Changes:
- fixed license (Classpath exception)
- removed "javafx.animation.fullspeed" in the test setup
- use 100 tests by default to shorten the test duration (but I kept the
high timeout values if the following parameter is increased):
I added the system property "ClipShapeTest.numTests" but it requires a
build.gradle change to pass the parameter:
+ // Marlin ClipShapeTest+
systemProperty "ClipShapeTest.numTests",
project.getProperty("ClipShapeTest.numTests")+
If it is not recommended to add such specific parameters into the
build.gradle file, what do you recommend ? (manual edit ?)
Best regards,
Laurent
>
> On 6/29/2018 11:25 AM, Kevin Rushforth wrote:
>
>> One more thing about the test. All of the OpenJFX unit tests should have
>> GPL v2 + Classpath Exception (this differs from the JDK).
>>
>> -- Kevin
>>
>>
>> On 6/29/2018 10:23 AM, Kevin Rushforth wrote:
>>
>>>
>>> I'll plan to review the code today if possible. This will need one more
>>> reviewer, so maybe Phil can also review it, since he reviewed the Java2D
>>> patch?
>>>
>>> As for my comments on the test:
>>>
>>> Finally I think this test should be manually run only if Marlin renderer
>>>> is modified.
>>>> How to do that ? use @Ignore or specific tags ?
>>>>
>>>
>>> As a slight variation of this: How about running a small number (say,
>>> 200 or 250) by default, but adding a flag to run more? Alternatively, you
>>> could use a flag to enable it, but since you would need to do something
>>> extra (provide a flag or modify the test) to run it, we might as well get
>>> at least some testing all the time. Unless you really think there is no
>>> value in doing this.
>>>
>>> I deliberately set all these Marlin clip (runtime + always subdivider) /
>>>> curve quality settings (quads / cubics thresholds) to be sure of the
>>>> concrete Marlin setup as quality thresholds are sensitive to such values.
>>>>
>>>
>>> As a best practice, tests should generally be run using the same
>>> settings as are used in production. Other than to verify how it behaves
>>> when you change these settings, I don't see the value in testing the system
>>> running in a mode that no application will ever see. I may be missing some
>>> point here.
>>>
>>> -- Kevin
>>>
>>>
>>> On 6/25/2018 9:01 AM, Laurent Bourgès wrote:
>>>
>>>> Kevin,
>>>>
>>>> Here are my comments below:
>>>>
>>>> 2018-06-16 1:47 GMT+02:00 Kevin Rushforth <kevin.rushforth at oracle.com
>>>> <mailto:kevin.rushforth at oracle.com>>:
>>>>
>>>> I tested this on all three platforms and the updated rasterizer
>>>> looks good.
>>>>
>>>> I spot checked the code changes, but didn't get time to do a
>>>> complete review yet. I was mostly looking for diffs between the
>>>> Java2D version which was already reviewed, and this one.
>>>>
>>>> I do have a couple comments on the new ClipShapeTest (which looks
>>>> like a nice accuracy test, btw).
>>>>
>>>> 1. The test runs for way too long (about 20x too long) to include
>>>> in our normal test runs. By default the entire class file (all
>>>> three tests) needs to take < 5 minutes and 2 minutes would be
>>>> better. I measured the time on 4 machines that I have and found
>>>> that if you cut the number of iterations down from 5000 to 250 it
>>>> will be just about the right run time. Then you can set the
>>>> timeout to 120 seconds (the slowest test on the slowest of my
>>>> machines took about 48 seconds, so a 2 minute timeout should be
>>>> plenty).
>>>>
>>>>
>>>> I agree this test is very long but it is the only mean I found to test
>>>> all possible stroke combinations and test enough shapes (5000) to detect
>>>> bugs.
>>>> I wondered if using mask directly (via ShapeUtils.getMaskData()) would
>>>> become faster but it will never run below the 2 minutes threshold in total.
>>>>
>>>> Finally I think this test should be manually run only if Marlin
>>>> renderer is modified.
>>>> How to do that ? use @Ignore or specific tags ?
>>>>
>>>>
>>>> 2. Can you explain the reason for setting the following?
>>>>
>>>> 206 // disable static clipping setting:
>>>> 207 System.setProperty("prism.marlin.clip", "false");
>>>> 208
>>>> System.setProperty("prism.marlin.clip.runtime.enable", "true");
>>>> 209
>>>> 210 // enable subdivider:
>>>> 211 System.setProperty("prism.marlin.clip.subdivider",
>>>> "true");
>>>> 212
>>>> 213 // disable min length check: always subdivide curves
>>>> at clip edges
>>>> 214 System.setProperty("prism.marlin.clip.subdivider.minLength",
>>>> "-1");
>>>> 215
>>>> 216 // If any curve, increase curve accuracy:
>>>> 217 // curve length max error:
>>>> 218 System.setProperty("prism.marlin.curve_len_err", "1e-4");
>>>> 219
>>>> 220 // cubic min/max error:
>>>> 221 System.setProperty("prism.marlin.cubic_dec_d2", "1e-3");
>>>> 222 System.setProperty("prism.marlin.cubic_inc_d1",
>>>> "1e-4"); // or disabled ~ 1e-6
>>>> 223
>>>> 224 // quad max error:
>>>> 225 System.setProperty("prism.marlin.quad_dec_d2", "5e-4");
>>>>
>>>> It seems better to test with the default parameters (i.e., it
>>>> makes a better regression test that way).
>>>>
>>>>
>>>> I deliberately set all these Marlin clip (runtime + always subdivider)
>>>> / curve quality settings (quads / cubics thresholds) to be sure of the
>>>> concrete Marlin setup as quality thresholds are sensitive to such values.
>>>>
>>>> The ClipShapeTest is dedicated to test the clipper (+ subdivider) part
>>>> of the Marlin renderer.
>>>>
>>>>
>>>>
>>>> 3. Related to that, I think you should eliminate the following (I
>>>> don't recommend running functional tests with this set), although
>>>> since you don't do any animation, it probably doesn't matter.
>>>>
>>>> 227 System.setProperty("javafx.animation.fullspeed",
>>>> "true"); // full speed
>>>>
>>>>
>>>> I will remove it and see if the overall test is not slower.
>>>> Is Platform.runLater() impacted by such setting (latency of FX thread
>>>> -> Prism rendering thread ?) ?
>>>>
>>>> Laurent
>>>>
>>>>
>>>>
>>>> On 6/8/2018 7:28 AM, Laurent Bourgès wrote:
>>>>
>>>> Hi,
>>>>
>>>> Please review this large patch to upgrade MarlinFX to 0.9.2 in
>>>> OpenJFX11:
>>>> JBS: https://bugs.openjdk.java.net/browse/JDK-8198885
>>>> <https://bugs.openjdk.java.net/browse/JDK-8198885>
>>>> webrev:
>>>> http://cr.openjdk.java.net/~lbourges/marlinFX/marlinFX-092.0/
>>>> <http://cr.openjdk.java.net/%7Elbourges/marlinFX/marlinFX-092.0/>
>>>> <http://cr.openjdk.java.net/%7Elbourges/marlinFX/marlinFX-092.0/
>>>> <http://cr.openjdk.java.net/%7Elbourges/marlinFX/marlinFX-092.0/>>
>>>> PR: https://github.com/javafxports/openjdk-jfx/pull/96
>>>> <https://github.com/javafxports/openjdk-jfx/pull/96> (CI OK)
>>>>
>>>> This patch is almost identical to Marlin(2D) patch, see:
>>>> https://bugs.openjdk.java.net/browse/JDK-8198885
>>>> <https://bugs.openjdk.java.net/browse/JDK-8198885>
>>>>
>>>> I added the ClipShapeTest (ported to jfx) that compares shape
>>>> clipping (within threshold) and it works (within large
>>>> timeouts):
>>>> gradle -PFULL_TEST=true :system:test --tests
>>>> test.com.sun.marlin.ClipShapeTest
>>>>
>>>> Regards,
>>>> Laurent
>>>>
>>>>
>>>>
>>>>
>>>
>>
>
--
--
Laurent Bourgès
More information about the openjfx-dev
mailing list