Proxy.isProxyClass scalability

[Mandy Chung]

Hi Peter,

We both prefer the interface types as the subKey.    See my comment 
inlined below.

On 4/23/2013 11:58 PM, Peter Levart wrote:
> I developed two different approaches:
>
> 1. Key made of WeakReference-s to interface Class objects.
> Strong points:
> - no key aliasing, validation can be pushed entirely to slow-path
> - quick and scalable
> - less memory usage than original code for 0 and 1-interface proxies
> Weak points:
> - more memory usage for 2+ -interface proxies
> Latest webrev:
> http://dl.dropboxusercontent.com/u/101777488/jdk8-tl/proxy-wc-wi/webrev.02/index.html
> Comments:
>   I like this one. If 2+ -interface proxies are relatively rare and 
> you don't mind extra space consumption for them,  I would go with this 
> approach.

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.

>
> That's the sole reason for optimized: WekaReferece<Class> -> 
> WeakReference<Class> -> ... -> WeakReference<Class> linked list 
> instead of the WeakReference<Class>[] array approach. And it's not 
> using any recursion for equals/hashCode, so the peformance is 
> comparable to array approach and it doesn't suffer from 
> StackOverflowExceptions for lots of interfaces.

I'm not objecting to build its own linked list but I'm not convinced if 
it's worth the extra complexity (although it's simple, this adds 
KeyNode, MidKeyNode, Key1, KeyX classes).  For 1-interface proxy, you 
can make it one single weak reference as you have right now.  I would 
simply think the array approach for 2+ interface proxy is preferable and 
the performance is comparable as you noted.  Did I miss any important 
reason you chose the linked list approach?  If not, let's do the simple 
approach that will help the future maintainers of this code.

The change in Proxy.java looks good to me with the comment about the 
array vs custom linked list.

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.

I think we're very close of getting this pushed.

Below are my comments to follow up the discussion we had last night and 
this morning just for clarification.  We should be now on the same page.

> Original code is actualy using the following structure: 
> WeakHashMap<ClassLoader, HashMap<List<String>, WeakReference<Class>>>
> ... so no ClassLoader leak here...
>

Oops... somehow I thought the original code didn't make a weak reference 
of the proxy class. I should have looked at the original code before I 
replied :(

> The TwoLevelWeakCache is an analogous structure: 
> ConcurrentHashMap<WeakReference<ClassLoader>, 
> ConcurrentHashMap<SubKey, WeakReference<Class>>>
>
> The point I was trying to make is that it is not needed to register a 
> ReferenceQueue with WeakReference<Class> values and remove entries as 
> they are cleared and enqueued, because they will not be cleared until 
> the ClassLoader that defines the Class-es is GC-ed and the expunging 
> logic can work solely on the grounds of cleared and enqueued 
> WeakReference<ClassLoader> keys, which is what my latest webrev using 
> String-based sub-keys does (I have to update the other too).

Agree.   That makes sense.   I got confused with CacheKey and the 
subkey.  I am now reading the WeakCache code closely.  You got it right 
that the key (defining ClassLoader), subkey (individual interfaces), and 
value (proxy class) have to be weakly referenced to ensure that classes 
loaded by any classloader cached as a key are not strongly reachable not 
causing the class loader leak.

> Not the key (interfaces array) but the individual interface Class 
> object - each has to be separately wrapped with a WeakReference. 

Sorry I should have been more precise - that's indeed what I meant.

thanks
Mandy