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

Jim Graham james.graham at oracle.com
Wed Oct 10 20:21:54 UTC 2012


Have you done any performance testing to see if the new protections cost 
any performance?

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).

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.

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...

			...jim

On 10/10/12 12:30 PM, Clemens Eisserer wrote:
> Hi Jim,
>
> A new version of the patch is available at:
> http://cr.openjdk.java.net/~ceisserer/7105461/webrev.05/
>
>> If x or y are>  MAX_SHORT then you should probably just reject the operation
>> immediately, otherwise you end up with a bunch of math that should end up
>> doing a NOP, but it is tenuous if it actually succeeds in doing so.
>
> Done. Thanks for pointing that out.
>
>> Also, if x,y are<  MIN_SHORT then when you clamp them to MIN_SHORT they may
>> be>  x2,y2 and so when you clamp the dimensions you will end up clamping a
>> massively negative number to Ushort.  I don't have the code off hand to see
>> how the method deals with that, but it is probably not 100% kosher in this
>> case (not sure if the subtraction might end up looking like a huge positive
>> number).  Thus, it is probably better to reject the op if x2,y2 are<
>> MIN_SHORT as well rather than rely on the processing.
>> Also, rejecting the call here rather than relying on the outcome of the math
>> avoids adding NOP requests to the X protocol stream...
>
> This would have been handled by the width/height-check later (tested),
> however as you pointed out it's better to exit early.
> Would you prefer combining the MIN/MAX checks after calculating x2/y2
> to improve redability?
>
> However, I found another problem: clampToUShort returned a short, so
> when a dimension>  Short.MAX_VALUE was clamped, the check>=0 later
> saw some negative value and exited for a valid request. As for now
> fillRect is the only consumer, I took the liberty to change its return
> type to int.
>
> Thanks a lot for the detailed in-depth review (and you patience).
>
> - Clemens



More information about the 2d-dev mailing list