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

[Jim Graham]

Sounds good then...

			...jim

On 2/19/2016 2:06 PM, Laurent Bourgès wrote:
> Dear Jim,
>
>     With respect to the ArrayCacheHolder, 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?
>
>
> In terms of liveness, a SoftReference is living a lot longer than a
> WeakReference; typically in my benchmarks, SoftReferences are only
> cleared when they are no more used for a long time (gc time threshold)
> i.e. after a very long time !
> That's why I really want to have a SOFT/WEAK solution to maintain the
> RendererContext in memory longer (more useful ie higher reuse rate) than
> the ArrayCacheHolder (less useful).
>
>     (Perhaps WEAK is much more transient than SOFT?)
>
>
> Exactly: weak references are (in practice) cleared at each young gen GC
> cycle ie their associated memory is very short lived.
>
>     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...
>
>
> The current behaviour is working as expected i.e. intentional as I want
> to minimize the memory footprint:
> - RendererContext are kept in memory by a SoftReference = longer alive
> (to maximize their reuse)
> - ArrayCache are kept by a WeakReference = shorter alive (to reduce
> their footprint)
>
> Besides, ArrayCache contain larger primitive arrays that consumes a lot
> more memory than a RendererContext (larger cache) for large shapes /
> canvas that should happen less frequently so the reallocation overhead
> should remain small.
>
> Regards,
> Laurent
>
>     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>
>         <mailto: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
>
>
>
>
> --
> --
> Laurent Bourgès