Re: Review request for 8169294:

[Jim Graham]

On 11/7/2016 1:14 PM, Laurent Bourgès wrote:

The only difference in Path2D.java I noted is that the Java2D version has
an EXPAND_MIN which is 10, but you re-use INIT_SIZE, which is 20, here for
the same purpose.


You're right; I think I didn't want to add an extra constant but if you
prefer being more consistent, I can prepare another webrev.


In looking at it again, I don't think it matters since it would only 
ever be triggered if they created a path that intentionally had fewer 
than the default number of entries, so it makes more sense to always 
grow by at least the INIT_SIZE rather than a smaller number.


I think I like the new policy better than the one in Java2D...

...jim

Re: Review request for 8169294:

[Kevin Rushforth]

I'll review the test tomorrow (I'm a little backed up on my code reviews 
given the impending change to build with jigsaw JDK).


-- Kevin


Laurent Bourgès wrote:

Jim,

2016-11-07 21:05 GMT+01:00 Jim Graham >:


I'd like to see Kevin review the test as I'm not the best expert
on our JUnit framework.


I just added @Test annotations and kept the jtreg tags in header (for 
information).
I could add asserts but JUnit does intercept any thrown exception and 
reports a test failure in such case.


I managed to undestand how to run my test with gradle:
gradle :graphics:test  --tests test.com.sun.javafx.geom.Path2DGrowTest

It is passing and the reports indicates that the 2 new tests are OK 
(report in modules/javafx.graphics/build/reports/tests/index.html)
 



It looks like it is mostly just going to emit some printouts about
performance (using echo() and log()) and verify that we don't get
any ArrayBounds related exceptions (or worse, OOME)?


Exactly, it is passing as there is no runtime exception and the 
performance issue is fixed as indicated by the few logged lines.
 



The only difference in Path2D.java I noted is that the Java2D
version has an EXPAND_MIN which is 10, but you re-use INIT_SIZE,
which is 20, here for the same purpose.


You're right; I think I didn't want to add an extra constant but if 
you prefer being more consistent, I can prepare another webrev.


Laurent

Re: Review request for 8169294:

[Laurent Bourgès]

Jim,

2016-11-07 21:05 GMT+01:00 Jim Graham :

> I'd like to see Kevin review the test as I'm not the best expert on our
> JUnit framework.
>

I just added @Test annotations and kept the jtreg tags in header (for
information).
I could add asserts but JUnit does intercept any thrown exception and
reports a test failure in such case.

I managed to undestand how to run my test with gradle:
gradle :graphics:test  --tests test.com.sun.javafx.geom.Path2DGrowTest

It is passing and the reports indicates that the 2 new tests are OK (report
in modules/javafx.graphics/build/reports/tests/index.html)


>
> It looks like it is mostly just going to emit some printouts about
> performance (using echo() and log()) and verify that we don't get any
> ArrayBounds related exceptions (or worse, OOME)?
>

Exactly, it is passing as there is no runtime exception and the performance
issue is fixed as indicated by the few logged lines.


>
> The only difference in Path2D.java I noted is that the Java2D version has
> an EXPAND_MIN which is 10, but you re-use INIT_SIZE, which is 20, here for
> the same purpose.
>

You're right; I think I didn't want to add an extra constant but if you
prefer being more consistent, I can prepare another webrev.

Laurent

Re: Review request for 8169294:

[Jim Graham]

I'd like to see Kevin review the test as I'm not the best expert on our 
JUnit framework.


It looks like it is mostly just going to emit some printouts about 
performance (using echo() and log()) and verify that we don't get any 
ArrayBounds related exceptions (or worse, OOME)?


The only difference in Path2D.java I noted is that the Java2D version 
has an EXPAND_MIN which is 10, but you re-use INIT_SIZE, which is 20, 
here for the same purpose.


I think that's fine, but wanted to point it out so you could comment...

...jim

On 11/6/2016 1:01 PM, Laurent Bourgès wrote:

Hi,

Please review this Path2D fix improving its storage growth algorithm as
done in java2d:
JBS: https://bugs.openjdk.java.net/browse/JDK-8169294
Webrev: http://cr.openjdk.java.net/~lbourges/fx/path2D/

PS: I converted the former jtreg test to JUnit test for OpenJFX. I hope
it is correct, as I was not able to run this test within gradle (but it
works in netbeans)

Best regards,
Laurent

Review request for 8169294:

[Laurent Bourgès]

Hi,

Please review this Path2D fix improving its storage growth algorithm as
done in java2d:
JBS: https://bugs.openjdk.java.net/browse/JDK-8169294
Webrev: http://cr.openjdk.java.net/~lbourges/fx/path2D/

PS: I converted the former jtreg test to JUnit test for OpenJFX. I hope it
is correct, as I was not able to run this test within gradle (but it works
in netbeans)

Best regards,
Laurent