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

[Philip Race]

This all looks good to me. Sorry it took a long time to get to it.

One thing that currently doesn't matter and perhaps never will is
HardReference perhapscould also over-ride clear() :

public void clear() {
   super.clear();
   strongRef = null;
}

strongRef must no longer be final in this case.

But if anyone ever needs that they can make the change ..

So approved.

-phil.


On 2/20/16, 3:36 AM, 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/
>         <http://cr.openjdk.java.net/%7Elbourges/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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/2d-dev/attachments/20160223/0b440e59/attachment.html>