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]> 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 <[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
