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
