[OpenJDK 2D-Dev] Various fixes to pisces stroke widening code

Jim Graham james.graham at oracle.com
Fri Jul 30 10:08:23 UTC 2010


Just to clarify.  My second message that I just sent mostly contained 
some additional optimizations to consider for now or later, but this 
message below contained at least one (maybe 2) thing(s) that looked like 
a bug and a few maintenance issues that I think should be done before 
finalizing this set of changes...

			...jim

On 7/29/2010 5:27 PM, Jim Graham wrote:
> Hi Denis,
>
> The changes look fine and I'm moving on to Renderer.java...
>
> I'd make as many methods private as you can. It looks like all your new
> methods are private, but some old methods are still public even though I
> don't think they are accessed elsewhere (like addEdge()?). I think
> "private" is enough for Hotspot to inline so that should help
> performance a bit. I usually use "final", but I think "private" does the
> same thing.
>
> You should create constants for the indices into the struct to make the
> code more readable (and to simplify updating the EDGE layout):
>
> X0_OFF = 0
> Y0_OFF = 1
> // etc.
>
> I don't think you touch X0 and Y0 after initializing them and then using
> them for the first setCurY so you could just use them as the "curX" and
> "curY" and save 2 slots in the table.
>
> You initialize edges twice - once when it is declared, and once in the
> constructor. Note that the initialization where it is declared uses
> INIT_SIZE which is not a multiple of EDGE size anyway.
>
> It looks like there is a missing statement in moveTo to initialize
> this.x0...?
>
> I need some more time on it, but I thought I would send along these
> comments in the meantime...
>
> ...jim
>
> Denis Lila wrote:
>> Hello Jim.
>>
>> I made the changes you point out, except for your second point
>> (I don't have time to think about it right now).
>>
>> I stopped precomputing m00_2_m01_2 because it was only being
>> used in one place. But I guess it worth it to precompute it
>> if it's going to be used often (and computeOffset is called
>> for every line).
>>
>> The new webrev is at
>> http://icedtea.classpath.org/~dlila/webrevs/fpBetterAAv2/webrev/
>>
>> Thanks,
>> Denis.



More information about the 2d-dev mailing list