Hi Denis,
Well, I guess it's a good thing that Java2Demo had a path like that in
it - not a very common case, so it's good we found it!
The fix looks fine. It still seems like there is way more logic there
than is needed - hopefully if we can get rid of flips at some point,
much of it will
To the end of getting rid of flips - all they are used for is to
resize the crossings array, right? This is not a huge array that costs
a lot to resize - why not simply use a default array of, say, 30
elements and then resize it if we ever have more crossings than that?
Only very complicated
Hello.
Are you a registered OpenJDK developer?
I am now.
Can I go ahead and push it?
Regards,
Denis.
Hi Denis,
That's great! I just did a last minute double-check of your last
(final) webrevs to be sure.
Have you tested Java2Demo with these changes? I'd also run any
regression tests you can find with the changes. If there are no
problems there, then you are good to go to push it...
Hi Jim.
I didn't know about Java2Demo. If I did I would have run it sooner.
But I ran it a few hours ago, and everything looked fine (surprisingly
high fps too) until I got to the append test.
Apparently I introduced a bug when solving the 2 consecutive moveTos bug.
Basically, when there's a
Hello Jim.
I'd need to see a final webrev and approve it and then anyone with an
OpenJDK user id can push it.
The final webrev is
http://icedtea.classpath.org/~dlila/webrevs/fpBetterAAv2/webrev/
The only things that have changed since the last one you commented on
are the y == boundsMaxY - 1
Hello Jim.
I'm guessing the test for y == boundsMaxY-1 at line 470 could
probably also be deleted now (since it will be handled by the end
test when y reaches maxY)?
Indeed it can. I've done this, and also updated a few comments that
might have been misleading (they were written before
Hi Denis,
More thoughts on Renderer.java.
-- Skipping gaps (minor optimization) --
If there is a gap in the edges in Y, say if a path consists of two
subpaths, one that covers y=[0..10] and another that covers
y=[1000..1010], then I think you will iterate over each y value from 10
to 1000,
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
Hi Jim.
Thanks for all your suggestions. I fixed the edge array indexing
issue, the moveTo bug (not assigning x0), and the double
initialization issue. I also improved the emission of the last row
to what you said. The link is the same as the last one I sent.
I considered one more
Hi Denis,
It looks fine. Hopefully we can eventually figure out why the sorting
on the fly didn't pan out.
Denis Lila wrote:
Hi Jim.
Thanks for all your suggestions. I fixed the edge array indexing
issue, the moveTo bug (not assigning x0), and the double
initialization issue. I also
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
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
Hello Jim.
This one performs almost identically to what is already there
in openjdk6 and 7, since it's exactly what I sent for review
last week, but with all the changes you suggested implemented.
I would actually like to ask you to not look at either one of
them. First of all, there is an
Woohoo, Denis! I look forward to seeing the new version!
...jim
On 7/28/2010 5:51 AM, Denis Lila wrote:
Hello Jim.
This one performs almost identically to what is already there
in openjdk6 and 7, since it's exactly what I sent for review
last week, but with all the changes
Hi Denis,
Only some minor comments so far:
Stroker.java:
- Should det be precomputed and saved? (You calculate it in the
constructor anyway, but don't save it.)
- Should test for uniform scale be precomputed?
- (Is test for uniform scale too strict? Can a rotated uniform scale
use the
Hi Denis,
I'll try to get through both versions and see if I can find anything
that was hurting performance with your EdgeLists. I'm guessing that
this version was created because of the performance issues you found
with the EdgeList version? Does this perform more closely to the
existing
Hello Jim.
I implemented your can of worms idea. It works, and it got rid of the biasing.
I wasn't able to send a webrev, but there are many changes and a side by side
comparison would probably be useless, so I just attached the file. I hope this
is
ok.
I also implemented a better iterating
Hello again.
This attachmet is a can of worms implementation without all the fancy (and
slow)
iteration. It also includes all of the other suggestions you sent in your first
review of Dasher and Renderer last week (most importantly, the firstOrientation
issue, horizontal lines filtering, and
Hello Jim.
Thank you very much for taking the time to read through this.
91 - is there a reason to copy the dash array? Theoretically if this
object were used in an arbitrary fashion it is good sense to protect
against data changing out from under it, but in practice we only ever
feed it
Denis Lila wrote:
Hello Jim.
Thank you very much for taking the time to read through this.
169 - if origLen reaches the end of the dash exactly (the == case)
You're right, I should. I can't just replace = with == though,
because the results will be the same: in the equal case origLen
Hi Denis,
This is awesome! Thanks for doing this.
I agree with all of your comments below. Here are some thoughts on the
new code:
Dasher.java:
91 - is there a reason to copy the dash array? Theoretically if this
object were used in an arbitrary fashion it is good sense to protect
Hello again.
I know I promised this last week, and I'm sorry it's late, but some nasty
bugs popped up. The webrev is at:
http://icedtea.classpath.org/~dlila/webrevs/floatingPoint/webrev/
I took this opportunity to make some changes that aren't related to floating
point conversion, since this
Hi Denis,
float operations are pretty fast and they are usually done in a separate
part of the processor so the compiler can schedule a lot of bookkeeping
instructions to run in parallel with waiting for the results of the FP
instruction. In the end, they often end up being free if there are
Hello.
And, I just finished it. At least it compiled successfully. I'm sure there
are a few runtime bugs. I'll try to have a webrev out by today.
Regards,
Denis.
- Jim Graham james.gra...@oracle.com wrote:
Hi Denis,
float operations are pretty fast and they are usually done in a
Hello.
Is it ok if I do this? I already started working on it on Friday.
I think I should be done by tomorrow.
But yes, I agree that we should convert to floating point. As for
performance, there's also the fact that right now we're trading
one double multiplication for 2 casts to long, 1 long
26 matches
Mail list logo
