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
