[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