[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-2d-dev
Subject: Re: [OpenJDK 2D-Dev] RFR 8159093: Fix coding conventions in Marlin renderer
From: Phil Race <philip.race () oracle ! com>
Date: 2016-06-15 18:01:02
Message-ID: 576197DE.5010203 () oracle ! com
[Download RAW message or body]
+1
-phil.
On 06/13/2016 04:31 AM, Laurent Bourgès wrote:
>
> Jim,
>
> This MT issue concerns only statistics collection so I deliberately
> did not ensure thread safety to avoid a synchronized block as the code
> is still "safe".
>
> Anyway I can fix it in the next patch that will improve array cache
> and stats per rendering context.
>
> Laurent
>
> Le 11 juin 2016 01:59, "Jim Graham" <james.graham@oracle.com
> <mailto:james.graham@oracle.com>> a écrit :
> >
> > The getInstance MT issue could be filed as a separate follow-on bug
> if you want, in which case webrev.1 is good to go...
> >
> > ...jim
> >
> >
> > On 6/10/2016 2:54 PM, Jim Graham wrote:
> >>
> >> Thanks Laurent,
> >>
> >> Eeek, I hate the type[] syntax for declaring arrays, but I guess I have
> >> to grow with the times.
> >>
> >> One last thing I just noticed, RendererState.getInstance doesn't
> protect
> >> against MT access if multiple threads encounter the null instance case
> >> and all decide to make their own...
> >>
> >> ...jim
> >>
> >> On 6/10/2016 4:59 AM, Laurent Bourgès wrote:
> >>>
> >>> Jim,
> >>>
> >>> I fixed the issues you mentioned, see below.
> >>>
> >>> Here is the new webrev:
> >>> http://cr.openjdk.java.net/~lbourges/marlin/marlin-8159093.1/
> <http://cr.openjdk.java.net/%7Elbourges/marlin/marlin-8159093.1/>
> >>>
> >>> I also fixed the bracket position (int val[] => int[] val) in Helpers,
> >>> MarlinRenderingEngine, MarlinTileGenerator classes.
> >>>
> >>> My comments:
> >>>
> >>> 2016-06-10 1:48 GMT+02:00 Jim Graham <james.graham@oracle.com
> <mailto:james.graham@oracle.com>
> >>> <mailto:james.graham@oracle.com <mailto:james.graham@oracle.com>>>:
> >>>
> >>>
> >>> In RendererStats, lines 276,277 - is it better to convert to an
> >>> array (which is an inherently risky situation for a concurrent
> >>> collection due to the potential for the size changing between the
> >>> array allocation and the toArray), or to iterate the concurrent
> >>> collection directly? I realize that the toArray() method protects
> >>> against a short array, but is it any better than just directly
> >>> iterating which would deal with the concurrency automatically
> anyway
> >>> without having to allocate an array. One thing to note, if you
> >>> convert to an array and there is a concurrency issue then the
> array
> >>> may have a null entry to indicate "this is the end of the
> list", but
> >>> you don't look for that null entry. A simple "if rdrCtx==null
> >>> break;" statement would be enough to deal with that case.
> >>>
> >>>
> >>> I agree and adopted the simplest solution: iterate directly on the
> >>> concurrent queue.
> >>>
> >>>
> >>> MarlinConst.java - you added DO_FLUSH_STATS, but I don't see it
> >>> getting used anywhere...?
> >>>
> >>>
> >>> Exact; I removed it as it will be only used in the next patch.
> >>>
> >>>
> >>> MarlinRenderingEngine.java - it looks like you eliminated all uses
> >>> of mon_npi_currentSegment, but it is still created in
> >>> RendererStats...?
> >>>
> >>>
> >>> mon_npi_currentSegment removed in RendererStats.
> >>>
> >>>
> >>> Histogram.java - 2016 copyright
> >>>
> >>>
> >>> Fixed.
> >>>
> >>> Regards,
> >>> Laurent
>
[Attachment #3 (text/html)]
<html>
<head>
<meta content="text/html; charset=UTF-8" http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
<div class="moz-cite-prefix">+1<br>
<br>
-phil.<br>
<br>
On 06/13/2016 04:31 AM, Laurent Bourgès wrote:<br>
</div>
<blockquote
cite="mid:CAKjRUT4Kwy0PR7f=aJcvEgqgwbQjkZqKObBAhu-XEJOReZgEWA@mail.gmail.com"
type="cite">
<p dir="ltr">Jim,</p>
<p dir="ltr">This MT issue concerns only statistics collection so
I deliberately did not ensure thread safety to avoid a
synchronized block as the code is still "safe".</p>
<p dir="ltr">Anyway I can fix it in the next patch that will
improve array cache and stats per rendering context.</p>
<p dir="ltr">Laurent</p>
<p dir="ltr">Le 11 juin 2016 01:59, "Jim Graham" <<a
moz-do-not-send="true" \
href="mailto:james.graham@oracle.com">james.graham@oracle.com</a>> a écrit \
:<br> ><br>
> The getInstance MT issue could be filed as a separate
follow-on bug if you want, in which case webrev.1 is good to
go...<br>
><br>
> ...jim<br>
><br>
><br>
> On 6/10/2016 2:54 PM, Jim Graham wrote:<br>
>><br>
>> Thanks Laurent,<br>
>><br>
>> Eeek, I hate the type[] syntax for declaring arrays,
but I guess I have<br>
>> to grow with the times.<br>
>><br>
>> One last thing I just noticed,
RendererState.getInstance doesn't protect<br>
>> against MT access if multiple threads encounter the
null instance case<br>
>> and all decide to make their own...<br>
>><br>
>> ...jim<br>
>><br>
>> On 6/10/2016 4:59 AM, Laurent Bourgès wrote:<br>
>>><br>
>>> Jim,<br>
>>><br>
>>> I fixed the issues you mentioned, see below.<br>
>>><br>
>>> Here is the new webrev:<br>
>>> <a moz-do-not-send="true"
href="http://cr.openjdk.java.net/%7Elbourges/marlin/marlin-8159093.1/">http://cr.openjdk.java.net/~lbourges/marlin/marlin-8159093.1/</a><br>
>>><br>
>>> I also fixed the bracket position (int val[] =>
int[] val) in Helpers,<br>
>>> MarlinRenderingEngine, MarlinTileGenerator classes.<br>
>>><br>
>>> My comments:<br>
>>><br>
>>> 2016-06-10 1:48 GMT+02:00 Jim Graham <<a
moz-do-not-send="true" \
href="mailto:james.graham@oracle.com">james.graham@oracle.com</a><br> >>> \
<mailto:<a moz-do-not-send="true"
href="mailto:james.graham@oracle.com">james.graham@oracle.com</a>>>:<br>
>>><br>
>>><br>
>>> In RendererStats, lines 276,277 - is it better
to convert to an<br>
>>> array (which is an inherently risky situation
for a concurrent<br>
>>> collection due to the potential for the size
changing between the<br>
>>> array allocation and the toArray), or to
iterate the concurrent<br>
>>> collection directly? I realize that the
toArray() method protects<br>
>>> against a short array, but is it any better
than just directly<br>
>>> iterating which would deal with the concurrency
automatically anyway<br>
>>> without having to allocate an array. One thing
to note, if you<br>
>>> convert to an array and there is a concurrency
issue then the array<br>
>>> may have a null entry to indicate "this is the
end of the list", but<br>
>>> you don't look for that null entry. A simple
"if rdrCtx==null<br>
>>> break;" statement would be enough to deal with
that case.<br>
>>><br>
>>><br>
>>> I agree and adopted the simplest solution: iterate
directly on the<br>
>>> concurrent queue.<br>
>>><br>
>>><br>
>>> MarlinConst.java - you added DO_FLUSH_STATS,
but I don't see it<br>
>>> getting used anywhere...?<br>
>>><br>
>>><br>
>>> Exact; I removed it as it will be only used in the
next patch.<br>
>>><br>
>>><br>
>>> MarlinRenderingEngine.java - it looks like you
eliminated all uses<br>
>>> of mon_npi_currentSegment, but it is still
created in<br>
>>> RendererStats...?<br>
>>><br>
>>><br>
>>> mon_npi_currentSegment removed in RendererStats.<br>
>>><br>
>>><br>
>>> Histogram.java - 2016 copyright<br>
>>><br>
>>><br>
>>> Fixed.<br>
>>><br>
>>> Regards,<br>
>>> Laurent<br>
</p>
</blockquote>
<br>
</body>
</html>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic