Proxy.isProxyClass scalability

[Peter Levart]

On 01/24/2013 03:40 PM, Vitaly Davidovich wrote:
>
> Peter,
>
> I know this was brought up on your c-i mailing list thread, but it 
> really feels like a new boolean field in j.l.Class is the cleaner 
> solution (and infinitely scalable and doesn't require bookkeeping in 
> the Proxy class).  Is there really no existing alignment gap in 
> j.l.Class layout that a boolean can slide into?
>
There might be, I will check. The ClassValue is scalable too (check the 
benchmarks), and bookkeeping is performed in the 
ClassValue.ClassValueMap that is referenced from the Class - not in the 
Proxy class. Unfortunately the j.l.r.Proxy class is in another package 
from j.l.Class, so the solution with a simple boolean field would 
require JavaLangAccess or Unsafe acrobatics.

Another thing is this separate bug:

http://bugs.sun.com/view_bug.do?bug_id=7123493

To solve it, a reference field in j.l.ClassLoader or a hypothetical 
ClassLoaderValue implementation would be required. I looked at the bug 
reporter's solution (ConcurrentProxy). He guards the 
WeakHashMap<ClassLoader, ...> with a ReadWriteLock:

/*
* Find or create the proxy class cache for the class loader.
*/
Map<Object,Reference<Class>> cache;
try{
loaderToCacheLock.readLock().lock();
cache = loaderToCache.get(loader);
}finally{
loaderToCacheLock.readLock().unlock();
}
// window of opportunity here between locks that would result in 
duplicate put(..) call
if (cache == null) {
try{
loaderToCacheLock.writeLock().lock();
cache = new ConcurrentHashMap<Object,Reference<Class>>();
loaderToCache.put(loader, cache);
}finally{
loaderToCacheLock.writeLock().unlock();
}
}


That code is broken twice. First, it is not double-checking the 
existence of cache in the writeLock guarded section. That's not so 
serious and could be fixed.
The other fault is using ReadWriteLock.readLock() to guard the 
WeakHashMap.get(). This is dangerous, since WeakHashMap.get() is a 
mutating method (expunging stale entries).

I don't think fixing this bug without either:
- new field in ClassLoader or
- ClassLoaderValue or
- WeakConcurrentHashMap

...is possible. Since we don't have the later two at the moment, the 
best bet for it unfortunately seems to be the first solution.

Regards, Peter

> Sent from my phone
>
> On Jan 24, 2013 9:35 AM, "Peter Levart" <peter.levart at gmail.com 
> <mailto:peter.levart at gmail.com>> wrote:
>
>     On 01/24/2013 03:10 PM, Alan Bateman wrote:
>
>         On 24/01/2013 13:49, Peter Levart wrote:
>
>             Should I file a RFE first?
>
>         Sorry I don't have time at the moment to study the proposed
>         patch but just to mention that it has come up a few times, its
>         just that it never bubbled up to the top of anyone's list.
>         Here's the bug tracking it:
>
>         http://bugs.sun.com/view_bug.do?bug_id=7123493
>
>         -Alan.
>
>     I belive that is another bottleneck. It is mentioning the
>     Proxy.getProxyClass method which also uses synchronization for
>     maintaining a cache of proxy classes by request parameters. I
>     could as well try to fix this too in the same patch if there is
>     interest.
>
>     Regards, Peter
>