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.
- "end < bands.length" was added to the loop for the reason of overall loop
performance(+15%).
----- james.gra...@oracle.com wrote:
> On 11/13/14 2:56 PM, Sergey Bylokhov wrote:
> > Hi, Jim
> >
> >> Please revert all "iff => if" changes in Region - those are not
> typos.
> >
> > ok
> >
> >> Hand-coding would require adding a bunch of code for clipping,
> though
> >> it might be faster still by avoiding all of the "are we on a new
> row?"
> >> testing in appendSpan/endRow().
> >
> > Looks like we get a big drawimage performance improvement from the
> previous fix - 30%(which is 10000% for the current jdk code) So I will
> try to go deeper and eliminate the call to appendSpan()/endRow.
> >
> >>
> >> Why does line 272 add "+1" for ?endIndex? Other places simply use
> >> "#rows * 5".
> >
> > This is because of needSpace() implementation:
> > private void needSpace(int num) {
> > if (endIndex + num >= bands.length) { }
> > If we have only one row then we need 5 bytes and num will be 5 =>
> equal to the bands.length => unnecessary copyArray will occures.
> > I suppose this is because endIndex should be a valid index in the
> bands array.
>
> That is probably a bug. That should probably be just ">" since I
> don't
> see anywhere where we ever place anything in that last entry.
>
> >> Other places in the code use the "clipAdd" and "dimAdd" methods to
> >> avoid
> >> overflow. In this particular case we might have overflow for
> >> box[0-3].
> >> On the other hand, since appendSpan() clips against the bounds
> we'd
> >
> >> resulting spans to be missing or incorrect (though it will never
> >> exceed
> >> the indicated bounding box even with overflow occuring)".
> >
> > I do not clip edges intentionally, because I trust the output of
> TransformHelper, which I assume should validate possible overflow.
> I'll update a javadoc.
>
> Exactly, this falls more under the realm of "we're creating a new API
>
> and even though it is only currently used by TransformHelper and we
> know
> how that works, we should consider that this API might get re-used in
>
> the future in other cases". As I said, this code ensures that the
> Region is valid, it just might produce unexpected (i.e. incorrect)
> results if we ever overflow. TransformHelper is unlikely to ever
> produce values that are much outside of the 0-<couple thousand> range
>
> since it only generates drawable-bounded coordinates so we should be
> very far from overflow, though what if someone creates a BufferedImage
>
> that is 1<<30 in dimensions (with the 64-bit VM and a lot of RAM)?
>
> ...jim
