Proxy.isProxyClass scalability

[Mandy Chung]

Hi Peter,

Sorry for the delay.

On 4/20/2013 12:31 AM, Peter Levart wrote:
> Hi Mandy,
>
> I have another idea. Before jumping to implement it, I will first ask 
> what do you think of it. For example:
>
> - have an optimal interface names key calculated from interfaces.
> - visibility of interfaces and other validations are pushed to 
> slow-path (inside ProxyClassFactory.apply)
> - the proxy Class object returned from WeakCache.get() is 
> post-validated with a check like:
>
> for (Class<?> intf : interfaces) {
>     if (!intf.isAssignableFrom(proxyClass)) {
>         throw new IllegalArgumentException();
>     }
> }
> // return post-validated proxyClass from getProxyClass0()...
>
> I feel that Class.isAssignableFrom(Class) check could be much faster 
> and that the only reason the check can fail is if some interface is 
> not visible from the class loader.
>
> Am I correct?

The isAssignableFrom check should be correct for well-behaved class 
loaders [1].  However, for non well-behaved class loaders, I'm not 
absolutely confident that this is right.  The case that I was concerned 
is when intf.isAssignableFrom(proxyClass) returns true but the proxy 
class doesn't implement the runtime types (i.e. given interfaces).   
Precise check should be to validate if the given interfaces == the proxy 
interfaces implemented by the cached proxy class (i.e. 
proxyClass.getInterfaces()).

Class.isAssignableFrom method checks if the proxy class is a subtype of 
the interface and no class loading/resolution involved.  It's definitely 
much cheaper than Class.forName which involves checking if a class is 
loaded (class loader delegation and class file search if not loaded - 
which is not the case you measure).

The existing implementation uses the interface names as the key to the 
per-loader proxy class cache.   I think we should use the Class<?>[] as 
the key (your webrev.02).   I have quickly modified the version I sent 
you (simply change the Key class to use Class<?>[] rather than String[] 
but didn't change the concurrent map cache to keep weak references) and 
the benchmark shows comparable speedup.

> I just noticed the following (have been thinking of it already, but 
> then forgot) in original code:
>
> /*
>  512              * Note that we need not worry about reaping the 
> cache for
>  513              * entries with cleared weak references because if a 
> proxy class
>  514              * has been garbage collected, its class loader will 
> have been
>  515              * garbage collected as well, so the entire cache 
> will be reaped
>  516              * from the loaderToCache map.
>  517              */
>
>

Hmm... I think the original code has the class loader leak.  The 
WeakHashMap<ClassLoader, HashMap<List<String>, Class>> keeps a weak 
reference to the ClassLoader but a strong reference to the cache of the 
proxy classes that are defined by that class loader.  The proxy classes 
are not GC'ed and thus the class loader is still kept alive.


> Each ClassLoader maintains explicit hard-references to all Class 
> objects for classes defined by the loader. So proxy Class object can 
> not be GC-ed until the ClassLoader is GC-ed. So we need not register 
> the CacheValue objects in WeakCache with a refQueue. The expunging of 
> reverseMap entries is already performed with CacheKey when it is 
> cleared and equeued. There's no harm as it is, since the clean-up is 
> performed with all the checks and is idempotent, but it need not be 
> done for ClassValue objects holding weak references to proxy Class 
> objects.
>

As explained above, for the per-loader proxy class cache, both the key 
(interfaces) and the proxy class should be wrapped with weak reference.

In your revisions, you optimize for 0-interface and 1-interface proxy 
class.  What I hacked up earlier was just to use Class<?>[] as the key 
(need to make a copy of the array to prevent that being mutated during 
runtime) that is a simpler and straightforward implementation.  I didn't 
measure the footprint and compare the performance of your versions.  
Have you seen any performance difference which led you to make the 
recent changes?

Mandy
[1] http://docs.oracle.com/javase/specs/jvms/se7/html/jvms-5.html#jvms-5.3