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.gra...@oracle.com
<mailto:james.gra...@oracle.com>
<mailto:james.gra...@oracle.com
<mailto:james.gra...@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
