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

[Jim Graham]

Looks good to go.  And it would be good to file the performance issue 
for small numbers of rectangles in Jira...

			...jim

On 10/12/2012 1:24 PM, Clemens Eisserer wrote:
> 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
>