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.lev...@gmail.com
<mailto:peter.lev...@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
