[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-2d-dev
Subject: Re: [OpenJDK 2D-Dev] [11] Upgrade to Marlin renderer 0.9.1
From: Laurent_Bourgès <bourges.laurent () gmail ! com>
Date: 2018-03-27 20:21:22
Message-ID: CAKjRUT4DJdrAFtC2Lj0WUR2e_goCjzKmBpuTsp9Tvxr+drpm=Q () mail ! gmail ! com
[Download RAW message or body]
Hi Phil,
Thank you a lot for your review and your time on this long & complex review.
2018-03-26 22:50 GMT+02:00 Philip Race <philip.race@oracle.com>:
> Hi Laurent,
>
> It took me I at least 5 tries to get all the way through this.
>
>
I agree it was a complex patch (clipping) with many other small
improvements, sorry.
> I don't have any substantial comments, just a few clean up questions.
>
>
> What is this in Curve.java + DCurve.java ?
>
> Even if derivatives are totally useless for lines, I removed the condition
to reset these values anyway.
> + if (false) {+ dax = 0.0d;+ day = 0.0d;+ \
> dbx = 0.0d;+ dby = 0.0d;+ }
> In Renderer.java, should the commented line be removed ?
>
> + && ((Math.abs(ddx) + Math.abs(ddy) * _SCALE_DY) <= _INC_BND+// \
> && (Math.abs(ddx + dddx) + Math.abs(ddy + dddy) * _SCALE_DY) <= _INC_BND
> A similar pattern occurs a little later in the same file.
>
> +// || (Math.abs(ddx + dddx) + Math.abs(ddy + dddy) * _SCALE_DY) >= \
> _DEC_BND
> Fixed (removed lines)
> + static final float LEN_TH = MarlinProperties.getSubdividerMinLength();
>
> You really meant to name it LEN_TH and not LENGTH ?
>
> That was deliberate, I wanted to shorten LENGTH_THRESHOLD => LEN_TH, not
LENGTH.
There are a few TODO's I see but they seem to be more about later
clean up or optimisation
> so are probably all OK.
>
> I added few new TODO that I hope to fix later (nothing critical).
> So I am OK to push, and if you clean up any of the above first I don't need to see \
> a new webrev.
> Thank you again,
Laurent
>
> On 3/23/18, 2:03 PM, Laurent Bourgès wrote:
>
> Hi,
>
> Sorry to insist but I would like to get feedback on this Marlin patch soon
> before going forward on tile-size tuning in java2d accelerated pipelines.
>
> Laurent
>
> 2018-03-21 22:56 GMT+01:00 Laurent Bourgès <bourges.laurent@gmail.com>:
>
> > Hi,
> >
> > Here is the updated webrev:
> > http://cr.openjdk.java.net/~lbourges/marlin/marlin-091.1/
> >
> > Changes in MarlinProperties only:
> > - getTileSize_Log2() & getTileWidth_Log2(); 32x32 tiles ie default = 5
> > (log2)
> >
> > I hope it is good for now as tile settings are the same as in jdk9/10.
> >
> > Regards,
> > Laurent
> >
> >
> > 2018-03-21 21:44 GMT+01:00 Laurent Bourgès <bourges.laurent@gmail.com>:
> >
> > > Sergey,
> > >
> > > Le mer. 14 mars 2018 Ã 17:14, Sergey Bylokhov <
> > > Sergey.Bylokhov@oracle.com> a écrit :
> > >
> > > > On 13/03/2018 17:04, Sergey Bylokhov wrote:
> > > >
> > > > > I have started to look to this review, will run some closed tests and
> > > > > send a feedback soon.
> > > > >
> > > >
> > > > No issues found so far, +1.
> > >
> > >
> > > Thanks for your vote.
> > > I need another approval I suppose ?
> > >
> > > I will prepare another review asap reverting only tile size changes as
> > > using large tiles has performance drop on d3d & ogl that needs more work.
> > > It can be done later in follow-up issues.
> > >
> > > Phil do you agree the proposed plan ?
> > >
> > > Laurent
> > >
> >
> >
>
[Attachment #3 (text/html)]
<div dir="ltr"><div>Hi Phil,</div><div><br></div><div>Thank you a lot for your review \
and your time on this long & complex review.<br></div><div \
class="gmail_extra"><br><div class="gmail_quote">2018-03-26 22:50 GMT+02:00 Philip \
Race <span dir="ltr"><<a href="mailto:philip.race@oracle.com" \
target="_blank">philip.race@oracle.com</a>></span>:<br><blockquote \
class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc \
solid;padding-left:1ex">
<div text="#000000" bgcolor="#FFFFFF">
<pre><span class="m_-8918979805826122505new">Hi Laurent,
It took me I at least 5 tries to get all the way through \
this.</span></pre></div></blockquote><div><br></div><div>I agree it was a complex \
patch (clipping) with many other small improvements, \
sorry.</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 \
.8ex;border-left:1px #ccc solid;padding-left:1ex"><div text="#000000" \
bgcolor="#FFFFFF"><pre><span class="m_-8918979805826122505new"> I don't have any \
substantial comments, just a few clean up questions.
What is this in Curve.java + DCurve.java ?</span></pre></div></blockquote><div>Even \
if derivatives are totally useless for lines, I removed the condition to reset these \
values anyway.<br><span class="m_-8918979805826122505new"> </span></div><blockquote \
class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc \
solid;padding-left:1ex"><div text="#000000" bgcolor="#FFFFFF"><pre><span \
class="m_-8918979805826122505new">+ if (false) {</span> <span \
class="m_-8918979805826122505new">+ dax = 0.0d;</span> <span \
class="m_-8918979805826122505new">+ day = 0.0d;</span> <span \
class="m_-8918979805826122505new">+ dbx = 0.0d;</span> <span \
class="m_-8918979805826122505new">+ dby = 0.0d;</span> <span \
class="m_-8918979805826122505new">+ }
In Renderer.java, should the commented line be removed ?
</span></pre>
<pre><span class="m_-8918979805826122505new">+ && \
((Math.abs(ddx) + Math.abs(ddy) * _SCALE_DY) <= _INC_BND </span><span \
class="m_-8918979805826122505new">+// && (Math.abs(ddx + \
dddx) + Math.abs(ddy + dddy) * _SCALE_DY) <= _INC_BND</span></pre> A similar \
pattern occurs a little later in the same file.<br>
<pre><span class="m_-8918979805826122505new">+// || (Math.abs(ddx \
+ dddx) + Math.abs(ddy + dddy) * _SCALE_DY) >= \
_DEC_BND</span></pre></div></blockquote><div>Fixed (removed lines)<br></div><div> \
</div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc \
solid;padding-left:1ex"><div text="#000000" bgcolor="#FFFFFF"><pre>+ static \
final float LEN_TH = MarlinProperties.<wbr>getSubdividerMinLength();
You really meant to name it LEN_TH and not LENGTH ?</pre></div></blockquote><div>That \
was deliberate, I wanted to shorten LENGTH_THRESHOLD => LEN_TH, not \
LENGTH.</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 \
.8ex;border-left:1px #ccc solid;padding-left:1ex"><div text="#000000" \
bgcolor="#FFFFFF"><pre>There are a few TODO's I see but they seem to be more \
about later clean up or optimisation so are probably all \
OK.</pre></div></blockquote><div>I added few new TODO that I hope to fix later \
(nothing critical).<br></div><blockquote class="gmail_quote" style="margin:0 0 0 \
.8ex;border-left:1px #ccc solid;padding-left:1ex"><div text="#000000" \
bgcolor="#FFFFFF"><pre>So I am OK to push, and if you clean up any of the above first \
I don't need to see a new webrev.</pre></div></blockquote><div>Thank you \
again,</div><div><br></div><div>Laurent<br></div><div> </div><blockquote \
class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc \
solid;padding-left:1ex"><div text="#000000" bgcolor="#FFFFFF"><div><div class="h5"> \
<br> On 3/23/18, 2:03 PM, Laurent Bourgès wrote:
<blockquote type="cite">
<div dir="ltr">
<div>
<div>Hi,<br>
<br>
</div>
Sorry to insist but I would like to get feedback on this
Marlin patch soon before going forward on tile-size tuning in
java2d accelerated pipelines.<br>
<br>
</div>
Laurent<br>
<div class="gmail_extra"><br>
<div class="gmail_quote">2018-03-21 22:56 GMT+01:00 Laurent
Bourgès <span dir="ltr"><<a href="mailto:bourges.laurent@gmail.com" \
target="_blank">bourges.laurent@gmail.com</a>></span>:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px \
#ccc solid;padding-left:1ex"> <div dir="ltr">
<div>
<div>Hi,</div>
<div><br>
</div>
<div>Here is the updated webrev:<br>
<a \
href="http://cr.openjdk.java.net/%7Elbourges/marlin/marlin-091.1/" \
target="_blank">http://cr.openjdk.java.net/~lb<wbr>ourges/marlin/marlin-091.1/</a><br>
<br>
</div>
Changes in MarlinProperties only:<br>
- getTileSize_Log2() & getTileWidth_Log2(); 32x32
tiles ie default = 5 (log2)</div>
<div><br>
</div>
<div>I hope it is good for now as tile settings are the
same as in jdk9/10.<br>
</div>
<div><br>
</div>
<div>Regards,<br>
</div>
Laurent
<div>
<div class="m_-8918979805826122505h5"><br>
<div>
<div>
<div>
<div class="gmail_extra"><br>
<div class="gmail_quote">2018-03-21 21:44
GMT+01:00 Laurent Bourgès <span dir="ltr"><<a \
href="mailto:bourges.laurent@gmail.com" \
target="_blank">bourges.laurent@gmail.com</a>></span>:<br>
<blockquote class="gmail_quote" style="margin:0px 0px \
0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> <div dir="auto">
<div>Sergey,<span \
class="m_-8918979805826122505m_1868194370483225134gmail-"><br> <br>
<div class="gmail_quote">
<div dir="ltr">Le mer. 14 mars
2018 Ã 17:14, Sergey Bylokhov
<<a \
href="mailto:Sergey.Bylokhov@oracle.com" \
target="_blank">Sergey.Bylokhov@oracle.com</a>>
a écrit :<br>
</div>
<blockquote class="gmail_quote" \
style="margin:0px 0px 0px 0.8ex;border-left:1px solid \
rgb(204,204,204);padding-left:1ex">
<div \
class="m_-8918979805826122505m_1868194370483225134gmail-m_-648558821017890324quoted-text">On
13/03/2018 17:04, Sergey
Bylokhov wrote:<br>
<blockquote class="gmail_quote" \
style="margin:0px 0px 0px 0.8ex;border-left:1px solid \
rgb(204,204,204);padding-left:1ex"> I have started to look to
this review, will run some
closed tests and send a
feedback soon.<br>
</blockquote>
<br>
</div>
No issues found so far, +1.</blockquote>
</div>
</span></div>
<div dir="auto"><br>
</div>
<div dir="auto">Thanks for your vote.</div>
<div dir="auto">I need another
approval I suppose ?</div>
<div dir="auto"><br>
</div>
<div dir="auto">I will prepare another
review asap reverting only tile size
changes as using large tiles has
performance drop on d3d & ogl
that needs more work. It can be done
later in follow-up issues.</div>
<div dir="auto"><br>
</div>
<div dir="auto">Phil do you agree the
proposed plan ?</div>
<span \
class="m_-8918979805826122505m_1868194370483225134gmail-HOEnZb"><font \
color="#888888"> <div dir="auto"><br>
</div>
<div dir="auto">Laurent</div>
</font></span></div>
</blockquote>
</div>
<br>
</div>
</div>
</div>
</div>
</div>
</div>
</div>
</blockquote>
</div>
<br>
</div>
</div>
</blockquote></div></div></div></blockquote></div></div></div>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic