[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-2d-dev
Subject: Re: [OpenJDK 2D-Dev] sun.java2D.Pisces renderer Performance and Memory enhancements
From: Laurent_Bourgès <bourges.laurent () gmail ! com>
Date: 2013-04-30 16:29:11
Message-ID: CAKjRUT5cz2wBE7HkYk1Jnv8L78YGKDepAgGefOoyHZ_-4VmVgg () mail ! gmail ! com
[Download RAW message or body]
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@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
[Attachment #3 (text/html)]
Jim,<br><br>here is the latest webrev:<br><a \
href="http://jmmc.fr/~bourgesl/share/java2d-pisces/webrev-3/">http://jmmc.fr/~bourgesl/share/java2d-pisces/webrev-3/</a><br><br>I \
tried to revert some syntax mistakes (indentation, moved constants + cleanup) : code \
cleanup is still in progress;<br> so please don't have a frawny face while \
looking at the code!<br><br>I fixed also Ductus and Jules rendering engines \
(TileState added in interface)<br><br>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: <br> 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]...<br><br>I have few questions regarding renderer \
edge handling and float ceil calls:<br> private void addLine(float x1, float y1, \
float x2, float y2) {<br>...<br> // LBO: why ceil ie upper integer \
?<br> final int firstCrossing = Math.max(ceil(y1), boundsMinY); // \
upper integer<br> final int lastCrossing = Math.min(ceil(y2), \
boundsMaxY); // upper integer<br> <br>=> firstCrossing / lastCrossing should use \
lower and upper integer values (rounding issues) ?<br><br> boolean \
endRendering() {<br> // TODO: perform shape clipping to avoid dealing \
with segments out of bounding box <br> <br> // Ensure shape edges are \
within bbox:<br> if (edgeMinX > edgeMaxX || edgeMaxX < 0f) {<br> \
return false; // undefined X bounds or negative Xmax<br> }<br> \
if (edgeMinY > edgeMaxY || edgeMaxY < 0f) {<br> return false; // undefined Y \
bounds or negative Ymax<br> }<br><br> // why use upper \
integer for edge min values ?<br>=> Here is the question: why use ceil instead of \
floor ?<br> <br> final int eMinX = ceil(edgeMinX); // \
upper positive int<br> if (eMinX > boundsMaxX) {<br> return \
false; // Xmin > bbox maxX<br> }<br><br> final int \
eMinY = ceil(edgeMinY); // upper positive int<br> if (eMinY > \
boundsMaxY) {<br> return false; // Ymin > bbox maxY<br> \
}<br><br> int spminX = Math.max(eMinX, boundsMinX);<br> \
int spmaxX = Math.min(ceil(edgeMaxX), boundsMaxX); // upper positive int<br> \
int spminY = Math.max(eMinY, boundsMinY);<br> int spmaxY = \
Math.min(ceil(edgeMaxY), boundsMaxY); // upper positive int<br> <br> \
int pminX = spminX >> SUBPIXEL_LG_POSITIONS_X;<br> int pmaxX = \
(spmaxX + SUBPIXEL_MASK_X) >> SUBPIXEL_LG_POSITIONS_X;<br> int \
pminY = spminY >> SUBPIXEL_LG_POSITIONS_Y;<br> int pmaxY = \
(spmaxY + SUBPIXEL_MASK_Y) >> SUBPIXEL_LG_POSITIONS_Y;<br> <br> \
// store BBox to answer ptg.getBBox():<br> this.cache.init(pminX, \
pminY, pmaxX, pmaxY);<br><br>In PiscesCache:<br> void init(int minx, int miny, \
int maxx, int maxy) {<br>...<br> // LBO: why add +1 ??<br> bboxX1 = \
maxx /* + 1 */;<br> bboxY1 = maxy /* + 1 */;<br><br>=> piscesCache \
previously added +1 to maximum (upper loop condition y < y1)<br>but the pmaxX \
already uses ceil (+1) and (spmaxY + SUBPIXEL_MASK_Y) ensures the last pixel is \
reached.<br> <br>I fixed these limits to have correct rendering due to dirty rowAARLE \
reuse.<br><br>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. <br> <br>Few \
comments:<br><br>2013/4/24 Jim Graham <span dir="ltr"><<a \
href="mailto:james.graham@oracle.com" \
target="_blank">james.graham@oracle.com</a>></span><br><div \
class="gmail_quote"><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt \
0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> <div class="im">On \
4/24/13 1:59 AM, Laurent BourgÄs wrote:<br> </div><div class="im"><blockquote \
class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid \
rgb(204,204,204);padding-left:1ex"> Jim,<br>
<br>
First, here are both updated webrev and benchmark results:<br>
- results: <a href="http://jmmc.fr/%7Ebourgesl/share/java2d-pisces/patch_opt_night.log" \
target="_blank">http://jmmc.fr/~bourgesl/<u></u>share/java2d-pisces/patch_opt_<u></u>night.log</a><br>
- webrev: <a href="http://jmmc.fr/%7Ebourgesl/share/java2d-pisces/webrev-2/" \
target="_blank">http://jmmc.fr/~bourgesl/<u></u>share/java2d-pisces/webrev-2/</a><br> \
</blockquote> <br></div>
Some comments on the webrev:<br>
<br>
- This caching of pipeline data in SG2D is a new precedent and I'm wary of it for \
a couple of reasons:<br>
- It gets tricky to satisfy all pipelines using that kind of technique<br>
- It breaks encapsulation, but at least it is isolated to internal code<br>
- SG2D will need to deal with the new field in clone().<br>
- 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<br>
- Thread local storage doesn't really have those issues<br>
- It's only used if that pipeline is used<br>
- No encapsulation issues<br>
- Don't need to modify clone() and it will work across \
clones<br></blockquote><div><br>Ok nevermind, I thought first it was possible but I \
agree it is not a proper solution.<br> </div><blockquote class="gmail_quote" \
style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid \
rgb(204,204,204);padding-left:1ex">
- Curve iterator uses auto-boxing, is that causing any \
churn?<br></blockquote><div>No, integers are in the Integer cache.<br> \
<br></div><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt \
0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
- RenderingEngine may want to provide "forwarding methods" for Ductus as a \
temporary solution until we fix Ductus to be aware of the new \
signatures<br></blockquote><div>Done in the new patch.<br> <br></div><blockquote \
class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid \
rgb(204,204,204);padding-left:1ex">
- new constants in Dasher were moved out of the class they are used \
from...?<br></blockquote><div>To be done (TBD)<br> <br></div><blockquote \
class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid \
rgb(204,204,204);padding-left:1ex">
- 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.<br> </blockquote><div>I have to fix it.<br> <br></div><blockquote \
class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid \
rgb(204,204,204);padding-left:1ex">
- 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]").<br> </blockquote><div>Agreed: TBD<br> <br></div><blockquote \
class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid \
rgb(204,204,204);padding-left:1ex">
- Perhaps it is time to get rid of the try/catch pairs in PTG.getAlpha() \
entirely?<br></blockquote><div>Done.<br> <br></div><blockquote class="gmail_quote" \
style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid \
rgb(204,204,204);padding-left:1ex">
- 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.<br> \
</blockquote><div>Fixed. <br></div><blockquote class="gmail_quote" style="margin:0pt \
0pt 0pt 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> <br>
- A lot of undoing of some very carefully planned indentation and code alignment \
issues. Auto-formatting is evil... ;)<br></blockquote><div>Sorry, I may tune \
netbeans formatting settings.<br> <br></div><blockquote class="gmail_quote" \
style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid \
rgb(204,204,204);padding-left:1ex">
- 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...)<br> </blockquote><div>Sorry again; however I like autoformating to let \
me work more on the code not on syntax / indentation. <br></div><blockquote \
class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid \
rgb(204,204,204);padding-left:1ex">
<br>
- We switched to a new "within" technique in the JavaFX version of Pisces \
based upon this paper:<br> <br>
<a href="http://www.cygnus-software.com/papers/comparingfloats/comparingfloats.htm" \
target="_blank">http://www.cygnus-software.<u></u>com/papers/comparingfloats/<u></u>comparingfloats.htm</a><br>
<br></blockquote><div>Good idea, but I think there is many place where float \
<-> int conversion / tests should be improved ...<br> <br>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).<br> </div></div><br>I have \
found few bugs:<br>- rendering a infinite line [vertical] draws nothing: I have to do \
a diagnostic ...<br>- clipping issues: <br>for very large dashed rectangle, millions \
of segments are emited from dasher (x1) to stroker (x4 segments) to renderer (x4 \
segments).<br> <br>However, I would like to add a minimal clipping algorithm but the \
rendering pipeline seems to be handling affine transforms between stages:<br> \
/*<br> * Pipeline seems to be:<br> * \
shape.getPathIterator <br>
* -> inverseDeltaTransformConsumer <br> * -> \
Dasher <br> * -> Stroker <br> * -> \
deltaTransformConsumer OR transformConsumer<br> * -> pc2d = \
Renderer (bounding box)<br>
*/<br><br>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 ...).<br><br>Could you give me advices how to hack / add \
clipping algorithm in the rendering pipeline (dasher / stroker / renderer) ?<br> \
<br>Should I try to derive a bounding box for the dasher / stroker depending on the \
Affine transforms ?<br><br>Laurent<br>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic