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. 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.
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.
I'm running more tests. If the above webrev looks fine with you, I'll
push the changeset tomorrow.
thanks
Mandy
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
