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

[Laurent Bourgès]

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> 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/
>>>
>>> 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>>:
>>>
>>>
>>>     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/20160613/0a2ee5bc/attachment.html>