[OpenJDK 2D-Dev] Please review patch for 7105461
james.graham at oracle.com
Fri Oct 12 21:48:37 UTC 2012
Looks good to go. And it would be good to file the performance issue
for small numbers of rectangles in Jira...
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).
> 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:
>> Have you done any performance testing to see if the new protections cost any
>> 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
More information about the 2d-dev