+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" <[email protected]
<mailto:[email protected]>> 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 <[email protected]
<mailto:[email protected]>
>>> <mailto:[email protected] <mailto:[email protected]>>>:
>>>
>>>
>>> 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
