[OpenJDK 2D-Dev] RFR 8148886: SEGV in sun.java2d.marlin.Renderer._endRendering

[Jim Graham]

Hi Laurent,

The code comment looks good now.

With respect to the ArrachCacheHolder, if the refType is SOFT, then you 
have a SOFT reference holding a WEAK reference.  Isn't that similar to 
the WEAK/WEAK case that you feel is redundant?  Or is there an important 
value to having a WEAK reference to the caches when the entire 
RendererContext is held by SOFT?

(Perhaps WEAK is much more transient than SOFT?)

I'm just verifying that you are intending that the Caches are held by a 
WEAK reference when the context is held by a SOFT reference. I don't 
really have a counter-argument other than it didn't make sense to me 
upon first glance, but if it is intentional than that's good enough for 
me. If that's true then I'm good with the fix...

			...jim

On 2/19/2016 12:14 AM, Laurent Bourgès wrote:
> Dear Jim,
>
> Please have a look to this updated webrev:
> http://cr.openjdk.java.net/~lbourges/marlin/marlin-8148886.6/
>
> I fixed the javadoc in ReentrantContextProvider: see below.
>
>
> 2016-02-18 23:40 GMT+01:00 Jim Graham <james.graham at oracle.com
> <mailto:james.graham at oracle.com>>:
>
>
>     I swear I thought I remember asking this before and getting an
>     answer, but I can't find it.
>
>     RenderContext line 49 - should that be "!= HARD" so that you keep a
>     hard reference for SOFT as well?
>
>
> This concerns the ArrayCacheHolder that gathers several ArrayCache
> instances that may consume a lot of memory: for example, it holds large
> primitive arrays in case of large shapes / images or very complex ones.
>
> To minimize the memory footprint, I deliberately use a
> WeakReference<ArrayCachesHolder>(holder) i.e. such large caches are
> "short live" (always in GC's young generation or TLAB ?).
>
> When the RendererContext is also kept in memory by a WeakReference, then
> it is redundant to have 2 WeakReferences:
> in this case, the forementioned statement set the USE_CACHE_HARD_REF to
> true that makes a strong reference between the RendererContext and its
> related ArrayCachesHolder.
>
> Finally I still plan to rewrite the "ArrayCache" feature as a more
> generic array cache as we discussed several times !
> Probably it is time to do it now or after fixing the bug:
> https://bugs.openjdk.java.net/browse/JDK-8144938
>
>     Also, I just noticed a typo in the sample code in
>     ReentrantContextProvider, shouldn't the constructor in the
>     newContext() method be "new ReentrantContextImpl()"?  (TileState
>     probably came from cutting and pasting from AAShapePipe.)
>
>
> Good catch: fixed.
>
> Besides I added a sample for ReentrantContextImpl:
>
>    52  * - create a subclass ReentrantContextImpl to hold the thread state:
>    53  *
>    54  * static final class ReentrantContextImpl extends ReentrantContext {
>    55  *     // specific cached data
>    56  * }
>    57  *
>
> Moreover I checked that the sample code is valid as it compiles properly
> in a new class.
>
>
> PS: I will be on winter holidays (leaving tomorrow for 1 week), so I
> would like to push this patch myself asap.
> If it is not possible, could you please handle the review thread & push
> it for me ?
>
> Cheers,
> Laurent