[OpenJDK 2D-Dev] [9] Review Request: 8059942 Default implementation of DrawImage.renderImageXform() should be improved for d3d/ogl

[Sergey Bylokhov]

Hi, Jim.
Please review an updated version of the fix:
http://cr.openjdk.java.net/~serb/8059942/webrev.18
I hope that all possible overflows/correctness/validity problems are 
checked now.

On 15.11.2014 1:50, Jim Graham wrote:
> Hi Sergey,
>
> On 11/14/14 6:40 AM, Sergey Bylokhov wrote:
>> Hi, Jim.
>> Please review an updated version of the fix:
>> http://cr.openjdk.java.net/~serb/8059942/webrev.17
>> Two notes:
>>   - one additional byte still is added to the bands, since I am still 
>> checking the code where we use endIndex-1/endIndex+1/endIndex++ or 
>> swap it with curYrow, etc. This isn't urgent.
>
> Since you no longer call appendSpan(), you no longer need the 
> additional byte in this new method.
>
>>   - "end < bands.length" was added to the loop for the reason of 
>> overall loop performance(+15%).
>
> That's odd that it helps since you then consume 5 entries after that 
> check, but I guess Hotspot works in mysterious ways.  I wonder if 
> "end+4" works as well or better since that would be a more accurate 
> test of what is happening.  Also, even for 15% performance difference 
> of this one method, it might be better to take the AAIOBE as a double 
> check against the caller feeding us bad data, such as edges[0,1] that 
> end up overflowing?  In either case, we generate valid data even in 
> the face of such anomalies.
>
> I'd still prefer to have the code confirm the ranges.  It would still 
> be much simpler than the old code that called appendSpan/endRow a lot 
> with just a few checks, like this:
>
>     static Region getInstance(final int lox, final int loy, final int 
> hix,
>                               final int hiy, final int edges[]) {
> // maybe?:        if (hiy <= loy || hix <= lox) return EMPTY_REGION;
>         final int y1 = loy + edges[0];
>         final int y2 = loy + edges[1];
>         // rowsNum * (3 + 1 * 2)
>         final int[] bands = new int[(y2 - y1) * 5];
>         int end = 0;
>         int index = 2;
> // Note to Sergey: Does end+4 also generate faster code?
>         for (int y = y1; end+4 < bands.length && y < y2; ++y) {
>             final int x1 = lox + edges[index++];
>             final int x2 = lox + edges[index++];
> // Is the following comment too obvious?
>             // Note: always consume both indices before rejecting span
>             if (y < loy || y >= hiy) continue;
>             if (x1 < lox) x1 = lox;
>             if (x2 > hix) x2 = hix;
>             if (x1 < x2) {
>                 bands[end++] = y;     // spanloy
>                 bands[end++] = y + 1; // spanhiy
>                 bands[end++] = 1;     // 1 span per row
>                 bands[end++] = x1;    // spanlox
>                 bands[end++] = x2;    // spanhix
>             }
>         }
>         return end != 0 ? new Region(lox, loy, hix, hiy, bands, end)
>                         : EMPTY_REGION;
>     }
>
> This code should ensure correctness/validity of the Region data 
> structure without much performance impact, hopefully?
>
>             ...jim


-- 
Best regards, Sergey.