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 <[email protected]
<mailto:[email protected]>>:
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
