peter.levart at gmail.com
Fri Apr 26 06:53:27 UTC 2013
Just a note on null keys (1st level)...
On 04/25/2013 11:53 PM, Mandy Chung wrote:
> Hi Peter,
> This looks great. I have imported your patch with slight modification
> in WeakCache:
> I believe WeakCache.get(K, P) should throw NPE if key is null and I
> fixed that.
The null key support is necessary to support bootstrap classloader as a
key. It could be supported by a separate 2nd-level map internally
(instead of using singleton NULL_KEY substitute Object), but the
external API must support null. And as I can see the webrev at above URL
still supports it.
> I changed refQueue to be of type ReferenceQueue<K> rather than
> ReferenceQueue<Object> since CacheValue no longer added to the ref
> queue. In the expungeStaleEntries method, change CacheKey<?> to
> CacheKey<K>. WeakCache.get(K, P) takes instance of K and P but
> subKeyFactory and valueFactory take superclasses of K and P - is that
> what you really want? I have changed them to BiFunction<K,P,...>. I
> also fixed a few typos and that's all.
It's just standard wildcards to allow functions with contra-varint
parameters and co-variant returns. It's useful as a generic API but in
our concrete usage doesn't have a value.
> The performance improvement is significant and I want to push this
> version to jdk8/tl. We can tune the memory usage in the future if
> that turns out to be an issue. I don't have plan to backport to
> jdk7u-dev unless there are customers escalating this :) This should
> be easy to convert without using BiFunction and Supplier and I will
> leave it as it is until there is a request to backport.
> I keep Key2 class since jdk also creates a proxy of 2 interfaces and
> you have already implemented it. If we think of a better way to
> replace the generation of the subkey in an optimized way, we could
> improve that in the future. The first and second level maps
> maintained in the WeakCache have Object as the type for the key which
> I think we should look for a specific type (CacheKey and SubKey
> type). To make the key of the first-level map to CacheKey would need
> to keep a separate cache for null key. For the second-level map, the
> subkey type would need to be declared as a parameter type to
> WeakCache. They are something that we should look at and clean up in
> the future if appropriate. I think what you have is good work that
> shouldn't be delayed any further.
The CacheKey is only private and internal to WeekCache, so making the
1st-level map's key as Object allows for NULL_KEY trick and makes logic
more uniform. If bootstrap classloader is used a lot to define proxy
classes, then a separate map can be viewed as a little speed-up for a
special case though (saves one Map.get, but introduces complications
with lazy instantiation - with AtomicReference perhaps). The sub-key as
a type parameter would only have some value if we would want the user of
WeakCache to constrain himself on the type of sub-keys returned by the
supplied subKeyFunction - so the constraint (the type of sub-keys) would
be specified together with the constrained function - at the WeakCache
constructor call site. In our case we would have to instantiate it as
Object (the common supertype of key0, Key1, Key2 and KeyX). The type
parameter for sub-key has little value in general, since WeakCache only
needs them to be Objects and sub-keys are never "returned" from the
methods of the public API...
> I'm running more tests. If the above webrev looks fine with you, I'll
> push the changeset tomorrow.
> On 4/25/13 8:40 AM, Peter Levart wrote:
>> Hi Mandy,
>> Here's another update that changes the sub-key back to
>> WeakReference<Class> based:
>> On 04/25/2013 03:38 AM, Mandy Chung wrote:
>>> I like this one too. The mapping is straight forward and clean that
>>> avoids the fallback and post validation path. Let's proceed with
>>> this one. It's good to optimize for the common case (1-interface).
>>> For 2 or more interfaces, we can improve the memory usage in the
>>> future when it turns out to be an issue. I'm fine with the
>>> zero-interface proxy which is the current implementation too.
>> I made 3 straight-forward implementations of sub-keys:
>> - Key1 - single interface
>> - Key2 - two interfaces
>> - KeyX - any number of interfaces
>> The cache-structure size increment for each new cached proxy-class is
>> (32 bit OOPS):
>> #of intfcs original patched
>> ---------- -------- ------------
>> 1 152 128(Key1)
>> 2 152 168(Key2), 208(KeyX)
>> 3 160 248(KeyX)
>> 4 160 280(KeyX)
>> ...so you can decide if Key2 is worth having or not.
>>> The javadoc for the get(K,P) method - @throws RuntimeException
>>> and @throws Error are not needed here since any method being called
>>> in the implementation may throw unchecked exceptions. There are a
>>> couple places that checks if (reverseMap != null) .... that check is
>>> not needed since it's always non-null. But I'm fine if you keep it
>>> as it is - just wanted to mention it in case it was just leftover
>>> from the previous version.
>> Removed the unneeded @throws and reverseMap != null checks (the later
>> was already removed in latest string-based webrev and I used that
>> version here).
>>> I think we're very close of getting this pushed.
>> Do you think this could also get backported to JDK7? The WeakCache
>> uses two interfaces from java.util.function. Should we make private
>> equivalents in this patch or do we leave that for the possible
>> back-porting effort. I should note that JDK7's ConcurrentHashMap is
>> not that space-efficient as proposed JDK8's, so space-usage would be
>> different on JDK7...
>> Regards, Peter
More information about the core-libs-dev