[OpenJDK 2D-Dev] RFR 8159093: Fix coding conventions in Marlin renderer

[Phil Race]

+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 at oracle.com 
> <mailto:james.graham at 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 at oracle.com 
> <mailto:james.graham at oracle.com>
> >>> <mailto:james.graham at oracle.com <mailto:james.graham at 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
>

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