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

[Phil Race]

Looks good : +1

-phil.

On 11/20/2014 11:11 PM, Sergey Bylokhov wrote:
> Thanks, Jim!
> Can anybody make the second review of this fix?
>
> Also I need a review of this change as well:
> http://mail.openjdk.java.net/pipermail/2d-dev/2014-November/004937.html
>
> On 20.11.2014 7:01, Jim Graham wrote:
>> Looks good...
>>
>>             ...jim
>>
>> On 11/19/14 11:44 AM, Sergey Bylokhov wrote:
>>> 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
>>>
>>>
>
>