[11] JDK-8204621: Upgrade MarlinFX to 0.9.2
Kevin Rushforth
kevin.rushforth at oracle.com
Tue Jul 3 12:08:25 UTC 2018
Hi Laurent,
> I added the system property "ClipShapeTest.numTests" but it requires
a build.gradle change to pass the parameter:
Yes, something like this is what I had in mind. As long as we don't add
too many of these, it is OK with me. Note that as coded, the build will
fail if you don't define ClipShapeTest.numTests, so you will need to
check for that. I note also that you used tabs in build.gradle (so
please change them to spaces). I recommend the following logic:
if (rootProject.hasProperty("ClipShapeTest.numTests")) {
systemProperty "ClipShapeTest.numTests",
rootProject.getProperty("ClipShapeTest.numTests")
}
-- Kevin
On 7/3/2018 2:13 AM, Laurent Bourgès wrote:
> Kevin,
>
> Thanks for the review !
>
> 2018-06-29 22:52 GMT+02:00 Kevin Rushforth <kevin.rushforth at oracle.com
> <mailto: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/
> <http://cr.openjdk.java.net/%7Elbourges/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>
> <mailto: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>
> <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/>>
> <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/
> <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>
> <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>
> <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