[11] JDK-8204621: Upgrade MarlinFX to 0.9.2
Kevin Rushforth
kevin.rushforth at oracle.com
Fri Jun 29 17:23:08 UTC 2018
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
>
>
>
More information about the openjfx-dev
mailing list