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

[Laurent Bourgès]

Jim,

here is the latest webrev:
http://jmmc.fr/~bourgesl/share/java2d-pisces/webrev-3/

I tried to revert some syntax mistakes (indentation, moved constants +
cleanup) : code cleanup is still in progress;
so please don't have a frawny face while looking at the code!

I fixed also Ductus and Jules rendering engines (TileState added in
interface)

In this patch, I modified the rowAARLE[][] array to only contain a single
tile line [32][4096] (512K). Each line is quite big already because I want
to store there the alpha data instead of RLE values:
RLE values are very interesting for horizontal lines but for dashed lines
it becomes bigger than alpha data. For example a dashed line [2-1] will
produce many segments that will be encoding like [01][x2]...

I have few questions regarding renderer edge handling and float ceil calls:
    private void addLine(float x1, float y1, float x2, float y2) {
...
        // LBO: why ceil ie upper integer ?
        final int firstCrossing = Math.max(ceil(y1), boundsMinY); // upper
integer
        final int lastCrossing  = Math.min(ceil(y2), boundsMaxY); // upper
integer

=> firstCrossing / lastCrossing should use lower and upper integer values
(rounding issues) ?

    boolean endRendering() {
        // TODO: perform shape clipping to avoid dealing with segments out
of bounding box

        // Ensure shape edges are within bbox:
        if (edgeMinX > edgeMaxX || edgeMaxX < 0f) {
            return false; // undefined X bounds or negative Xmax
        }
        if (edgeMinY > edgeMaxY || edgeMaxY < 0f) {
            return false; // undefined Y bounds or negative Ymax
        }

        // why use upper integer for edge min values ?
=> Here is the question: why use ceil instead of floor ?

        final int eMinX = ceil(edgeMinX); // upper positive int
        if (eMinX > boundsMaxX) {
            return false; // Xmin > bbox maxX
        }

        final int eMinY = ceil(edgeMinY); // upper positive int
        if (eMinY > boundsMaxY) {
            return false; // Ymin > bbox maxY
        }

        int spminX = Math.max(eMinX, boundsMinX);
        int spmaxX = Math.min(ceil(edgeMaxX), boundsMaxX); // upper
positive int
        int spminY = Math.max(eMinY, boundsMinY);
        int spmaxY = Math.min(ceil(edgeMaxY), boundsMaxY); // upper
positive int

        int pminX = spminX >> SUBPIXEL_LG_POSITIONS_X;
        int pmaxX = (spmaxX + SUBPIXEL_MASK_X) >> SUBPIXEL_LG_POSITIONS_X;
        int pminY = spminY >> SUBPIXEL_LG_POSITIONS_Y;
        int pmaxY = (spmaxY + SUBPIXEL_MASK_Y) >> SUBPIXEL_LG_POSITIONS_Y;

        // store BBox to answer ptg.getBBox():
        this.cache.init(pminX, pminY, pmaxX, pmaxY);

In PiscesCache:
    void init(int minx, int miny, int maxx, int maxy) {
...
        // LBO: why add +1 ??
        bboxX1 = maxx /* + 1 */;
        bboxY1 = maxy /* + 1 */;

=> piscesCache previously added +1 to maximum (upper loop condition y < y1)
but the pmaxX already uses ceil (+1) and (spmaxY + SUBPIXEL_MASK_Y) ensures
the last pixel is reached.

I fixed these limits to have correct rendering due to dirty rowAARLE reuse.

I think that the thread local's RendererContext is now small enough (< 1
Mb) to be used by web containers but if the number of threads is very high,
concurrent queue could help minimizing wasted memory.

Few comments:

2013/4/24 Jim Graham <james.graham at oracle.com>

> 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<http://jmmc.fr/%7Ebourgesl/share/java2d-pisces/patch_opt_night.log>
>> - webrev: http://jmmc.fr/~bourgesl/**share/java2d-pisces/webrev-2/<http://jmmc.fr/%7Ebourgesl/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
>

Ok nevermind, I thought first it was possible but I agree it is not a
proper solution.


> - Curve iterator uses auto-boxing, is that causing any churn?
>
No, integers are in the Integer cache.


> - RenderingEngine may want to provide "forwarding methods" for Ductus as a
> temporary solution until we fix Ductus to be aware of the new signatures
>
Done in the new patch.


> - new constants in Dasher were moved out of the class they are used
> from...?
>
To be done (TBD)


> - 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.
>
I have to fix it.


> - 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]").
>
Agreed: TBD


> - Perhaps it is time to get rid of the try/catch pairs in PTG.getAlpha()
> entirely?
>
Done.


> - 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.
>
Fixed.

>
> - A lot of undoing of some very carefully planned indentation and code
> alignment issues.  Auto-formatting is evil... ;)
>
Sorry, I may tune netbeans formatting settings.


> - 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...)
>
Sorry again; however I like autoformating to let me work more on the code
not on syntax / indentation.

>
> - 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<http://www.cygnus-software.com/papers/comparingfloats/comparingfloats.htm>
>
> Good idea, but I think there is many place where float <-> int conversion
/ tests should be improved ...

Do you have any faster implementation for Math.ceil/floor ? I now use
casting 1 + (int) / (int) but I know it is only correct for positive
numbers (> 0).

I have found few bugs:
- rendering a infinite line [vertical] draws nothing: I have to do a
diagnostic ...
- clipping issues:
for very large dashed rectangle, millions of segments are emited from
dasher (x1) to stroker (x4 segments) to renderer (x4 segments).

However, I would like to add a minimal clipping algorithm but the rendering
pipeline seems to be handling affine transforms between stages:
        /*
         * Pipeline seems to be:
         *    shape.getPathIterator
         * -> inverseDeltaTransformConsumer
         * -> Dasher
         * -> Stroker
         * -> deltaTransformConsumer OR transformConsumer
         * -> pc2d = Renderer (bounding box)
         */

It is complicated to perform clipping in the Renderer and maybe to late as
a complete stroker's segment must be considered as a whole (4 segments
without caps ...).

Could you give me advices how to hack / add clipping algorithm in the
rendering pipeline (dasher / stroker / renderer) ?

Should I try to derive a bounding box for the dasher / stroker depending on
the Affine transforms ?

Laurent
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/2d-dev/attachments/20130430/a1ff768c/attachment.html>