[OpenJDK 2D-Dev] Please review patch for 7105461

[Clemens Eisserer]

Hi Jim,

> One comment on code style - "if" is not a function call so the style
> guidelines specify a space before the parentheses with the conditional test
> (line 128 and 135).
Done.
Will keep style guidlines in mind next time.

> The logic looks fine, but I'll point out that width/height can never be < 0
> since they are the output from ClampToUShort so the test looks odd (but
> isn't incorrect).  You could test for simply == 0 or you could test for
> x2,y2 <= x,y before the ClampToUshort, but testing for <= 0 after certainly
> isn't hurting anything.
I changed it to == 0, to make the code more consistent.
Thanks for pointing this out.

Please find the final patch at:
http://cr.openjdk.java.net/~ceisserer/7105461/webrev.06/

> Have you done any performance testing to see if the new protections cost any
> performance?
>
> Also, many of these tests are duplicated between the shortcut tests in the
> method and the (hidden behind a method call) tests done by the clamp/clip
> methods.  I'm not sure that hurts performance, though, but if these changes
> show up on a microbenchmark then we might want to consider inlining all of
> the logic and eliminating duplicate tests.  For example, the ClampToUShort
> method tests for < 0 so it can clamp to 0, but the calling method will then
> short-cut on that same case anyway.  The code might be less obvious if it is
> all inlined, but fillRect is a fairly common operation that we don't want to
> impact too much simply for sake of having an obvious implementation...

I used J2DBench with client-jvm on amd64 to test 1x1 rectangles and
didn't find any measureable difference on my notebook (intel + SNA,
fastest driver for 2D as far as I know). Usually even hotspot-client
is quite good optimizing code like this, as the static methods will be
inlined and redundant checks are usually removed.

However, I did find 1x1 fillRect with the xrender pipeline to be a lot
slower compared to X11 which is caused by the xrender pipeline only
having a XRenderFillRectangles native method, which always calls
GetIntArrayCritical on a java array even for a single rectangle.
A fast-path for a single rectangle should pay off for small dimensions.

Thanks, Clemens