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
