On 4/24/13 1:59 AM, Laurent Bourgès wrote:
Jim,
First, here are both updated webrev and benchmark results:
- results: http://jmmc.fr/~bourgesl/share/java2d-pisces/patch_opt_night.log
- webrev: http://jmmc.fr/~bourgesl/share/java2d-pisces/webrev-2/
Some comments on the webrev:
- This caching of pipeline data in SG2D is a new precedent and I'm wary
of it for a couple of reasons:
- It gets tricky to satisfy all pipelines using that kind of technique
- It breaks encapsulation, but at least it is isolated to internal code
- SG2D will need to deal with the new field in clone().
- Rendering a hierarchy of Swing objects uses clone() a lot so
caching storage in SG2D may not help much in that case and thread local
may be more of a win
- Thread local storage doesn't really have those issues
- It's only used if that pipeline is used
- No encapsulation issues
- Don't need to modify clone() and it will work across clones
- Curve iterator uses auto-boxing, is that causing any churn?
- RenderingEngine may want to provide "forwarding methods" for Ductus as
a temporary solution until we fix Ductus to be aware of the new signatures
- new constants in Dasher were moved out of the class they are used from...?
- Why get rid of the final modifiers in Dasher? Was it not as effective
as copying to local variables? Note that the manual copying to local
variables is prone to maintenance issues if someone inserts a method
call in a block where the fields are stale.
- PRE.pathTo() could still be static, no? Also, it would be nice to
make this change to the existing RE helper method directly (and possibly
provide a backwards compatibility method with the old argument list that
simply forwards with a "new float[6]").
- Perhaps it is time to get rid of the try/catch pairs in PTG.getAlpha()
entirely?
- When you have a field cached in a local variable and you compute a new
value for it, then "field = var = ..." is probably better than "var =
field = ..." since the latter implies that the answer gets stored in the
field and then read back which is an extra memory operation. I noticed
this in a couple of places in Renderer, but I know I saw the local
variable caching in other files as well.
- A lot of undoing of some very carefully planned indentation and code
alignment issues. Auto-formatting is evil... ;)
- A personal rant - I'm a big fan of the { on a line by itself if it
follows an indented line-continued method declaration or control
statement. It helps the eye quickly find the start of the body because
it stands out. Your autoformatting got rid of a bunch of those and I
made a frowny face... :( (It may not be true to the JDK code style
guidelines, but we've been using that style throughout the 2D code for
years...)
- We switched to a new "within" technique in the JavaFX version of
Pisces based upon this paper:
http://www.cygnus-software.com/papers/comparingfloats/comparingfloats.htm
The C version of the implementation in that paper is:
// Usable AlmostEqual function
// See
http://www.cygnus-software.com/papers/comparingfloats/comparingfloats.htm
jboolean Helpers_withinULP(const jfloat A, const jfloat B, const int
maxUlps) {
// Make sure maxUlps is non-negative and small enough that the
// default NAN won't compare as equal to anything.
// assert(maxUlps > 0 && maxUlps < 4 * 1024 * 1024);
jint aInt, bInt;
// Make aInt lexicographically ordered as a twos-complement int
// This cast can induce "false positive" warnings from various
compilers
// or bug checking tools, but is correct as sizeof(jint) ==
sizeof(jfloat)
aInt = *((jint *) &A);
if (aInt < 0) {
aInt = 0x80000000 - aInt;
}
// Make bInt lexicographically ordered as a twos-complement int
// This cast can induce "false positive" warnings from various
compilers
// or bug checking tools, but is correct as sizeof(jint) ==
sizeof(jfloat)
bInt = *((jint *) &B);
if (bInt < 0) {
bInt = 0x80000000 - bInt;
}
// aInt,bInt are in the range [-0x7fffffff, +0x7fffffff]
// assuming maxUlps is much smaller than 0x7fffffff
// (<negative number> + maxUlps) will never overflow
// (<positive number> - maxUlps) will never overflow
if (aInt < bInt) {
return (aInt < 0) ? aInt + maxUlps >= bInt : bInt - maxUlps <=
aInt;
} else {
return (bInt < 0) ? bInt + maxUlps >= aInt : aInt - maxUlps <=
bInt;
}
}
It avoids multiple calls to ULP and the complexity of figuring out
whether to use the ULP of the larger or smaller number. The Java
version would have to use Float.floatToIntBits() where we used funny
pointer casting...
...jim
