[OpenJDK 2D-Dev] Please review patch for 7105461
linuxhippy at gmail.com
Fri Oct 12 20:24:20 UTC 2012
> 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.
More information about the 2d-dev