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. > > 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. > > ...jim > > On 11/13/14 8:35 AM, Sergey Bylokhov wrote: > > Hi, Jim. > > Please review an updated version of the fix: > > http://cr.openjdk.java.net/~serb/8059942/webrev.16 > > The new method in Region class was added. > > > > > > On 11.11.2014 5:43, Jim Graham wrote: > >> > >> > >> On 11/10/14 2:52 PM, Sergey Bylokhov wrote: > >>> Hi, Jim. > >>> To simply fill a span in the Region, I'll need to open > >>> Region.appendSpan() method, > >>> but I do not want to do it. > >> > >> No, that method should remain private. All region objects should > be > >> fully constructed at all times when in the hands of calling code. > >> > >>> So I need to create my own EdgesSpanIterator, or totally > eliminate > >> this loop and > >>> return Region object from the TransformHelper directly. > >> > >> No, all you need is a method: > >> > >> /** > >> * (describe edges array - 0,1 are y range, 2N,2N+1 are x ranges, > 1 > >> per y range) > >> * (dxoff, dyoff are offsets that the edges array values are > relative to) > >> */ > >> static Region getInstance(int dxoff, int dyoff, int edges[]) { > >> // copy loop from DrawImage.java here, but use direct > manipulation > >> // of the spans array instead of union operations. > >> // You should be able to pre-allocate the spans array as well > to > >> // save on growth using appendSpan() et al. > >> } > >> > >> That should take only a few minutes to write, then DrawImage simply > does: > >> > >> Region blitclip = Region.getInstance(dx1, dy1, edges); > >> > >>> Probably some logic can be moved from TransformHelper to the java > level > >>> (at least generation of edges array). Additionally it will be > simpler to > >>> check a performance improvement, because now it is not so > obvious > >> comparing > >>> to the current percents improvement. > >> > >> Unfortunately the exact calculations to compute the edges array > depend > >> on a lot of calculations done at the native level. This would be > a > >> major repartitioning of the code... > >> > >> ...jim > > > >