Proxy.isProxyClass scalability

[Mandy Chung]

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