Proxy.isProxyClass scalability (original) (raw)

Peter Levart peter.levart at gmail.com
Fri Apr 26 06:53:27 UTC 2013

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 rather than ReferenceQueue since CacheValue no longer added to the ref queue. In the expungeStaleEntries method, change CacheKey<?> to CacheKey. 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 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

More information about the core-libs-dev mailing list