[OpenJDK 2D-Dev] [11] Upgrade to Marlin renderer 0.9.1

[Laurent Bourgès]

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 at 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 at 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 at gmail.com>:
>>
>>> Sergey,
>>>
>>> Le mer. 14 mars 2018 à 17:14, Sergey Bylokhov <
>>> Sergey.Bylokhov at 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
>>>
>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/2d-dev/attachments/20180327/2d17c07f/attachment.html>