[OpenJDK 2D-Dev] sun.java2D.Pisces renderer Performance and Memory enhancements

[Jim Graham]

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