[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