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
