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

[Laurent Bourgès]

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>>:
>>
>>
>>
>>     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/20160219/83039719/attachment.html>