Hi Peter,
On 4/17/13 7:18 AM, Peter Levart wrote:
As expected, the Proxy.getProxyClass() is yet a little slower than
with FlattenedWeakCache, but still much faster than original Proxy
implementation. Another lookup in the ConcurrentHashMap and another
indirection have a price, but we also get something in return - space.
[...]
So we loose approx. 32 bytes (32bit addresses) or 48 bytes (64 bit
addresses) for each proxy class compared to original code when using
FlattenedWeakCache, but we gain 8 bytes (32 bit or 64 bit addresses)
for each proxy class cached compared to original code when using
TwoLevelWeakCache. So which to favour, space or time?
With TwoLevelWeakCache, there is a "step" of 108 bytes (32bit
addresses) when new ClassLoader is encoutered (new 2nd level
ConcurrentHashMap is allocated and new entry added to 1st level CHM.
There's no such "step" in FlattenedWeakCache (modulo the steps when
the CHMs are itself resized). So we roughly have 108 bytes wasted for
each new ClassLoader encountered with TwoLevelWeakCache vs.
FlattenedWeakCache, but we also have 40 bytes spared for each proxy
class cached. TwoLevelWeakCache starts to pay off if there are at
least 3 proxy classes defined per ClassLoader in average.
Thanks for the detailed measurement and analysis. Although the extra
lookup on the per-loader cache incurs additional overhead on
Proxy.getProxyClass, it still shows 10x speed on your performance
measurement result which is very good.
Since the performance improvement is significant on both approaches, the
memory saving would be the desirable choice.Especially Java SE 8
profiles [1] can run on small devices and we should be cautious about
the space and speed tradeoff.I'll support going for per-loader cache
(i.e. two-level weak cache).
Another point - Proxies are used in the JDK to implement annotations,
java.beans.EventHandler, JMX MXBeans mapping, JMX MBean proxies,
Invocable interface by script engines, and RMI/serialization. JAXWS
also uses proxies to support @javax.xml.bind.annotation.XmlElement. For
the case of annotations and JMX, the number of generated proxy classes
would typically not be a couple that depends on what interfaces
application define. In EE environment, there will be many class loaders
running. It'd be better to prepare to work the moderate number, if not
high, of proxy classes and multiple loaders case.
https://dl.dropboxusercontent.com/u/101777488/jdk8-tl/proxy-wc/webrev.02/index.html
What about package-private in java.lang.reflect? It makes Proxy itself
much easier to read. When we decide which way to go, I can remove the
interface and only leave a single package-private class...
thanks. Moving it to a single package-private classin
java.lang.reflectand remove the interface sounds good.
I have merged your patch with the recent TL repo and did some clean up
while reviewing it. Some comments:
1. getProxyClass0 should validate the input interfaces and throw IAE if
any illegal argument (e.g. interfaces are not visible to the given
loader) before calling proxyClassCache.get(loader, interfaces). I moved
back the validation code from ProxyClassFactory.apply to getProxyClass0.
2. I did some cleanup to restore some original comments to make the
diffs clearer where the change is.
3. I removed the newInstance method which is dead code after my previous
code. Since we are in this code, I took the chance to clean that up and
also change a couple for-loop to use for-each.
I have put the merged and slightly modified Proxy.java and webrev at:
http://cr.openjdk.java.net/~mchung/jdk8/webrevs/7123493/webrev.00/
We will use this bug for your contribution:
7123493 : (proxy) Proxy.getProxyClass doesn't scale under high load
For the weak cache class, since it's for proxy implementation use, I
suggest to take out the supportContainsValueOperation flagas it always
keeps the reverse map for isProxyClass lookup.
You can simply call Objects.requireNonNull(param) instead of
requireNonNull(param, "param-name") since the proxy implementation
should have made sure the argument is non-null.
Formatting nits:
68 Object cacheKey = CacheKey.valueOf(
69 key,
70 refQueue
71 );
should be: all in one line or line break on a long-line. Same for
method signature.
237 void expungeFrom(
238 ConcurrentMap<?, ? extends ConcurrentMap<?, ?>> map,
239 ConcurrentMap<?, Boolean> reverseMap
240 );
should be:
void expungeFrom(ConcurrentMap<?, ? extends ConcurrentMap<?, ?>> map,
ConcurrentMap<?, Boolean> reverseMap);
so that it'll be more consistent with the existing code. I'll do a
detailed review on the weak cache class as you will finalize the code
per the decision to go with the two-level weak cache.
Thanks again for the good work.
Mandy
[1] http://openjdk.java.net/jeps/161
