Proxy.isProxyClass scalability

[Peter Levart]

Hi Mandy,

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:
> http://cr.openjdk.java.net/~mchung/jdk8/webrevs/7123493/webrev.01/
>
> 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.
>
> thanks
> Mandy

Fingers crossed.

Regards, Peter

>
> 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:
>>
>> http://dl.dropboxusercontent.com/u/101777488/jdk8-tl/proxy-wc/webrev.05/index.html
>>
>> 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.
>>
>>>
>>> WeakCache.java
>>>    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
>>
>>>
>>>
>>> thanks
>>> Mandy
>>
>