Proxy.isProxyClass scalability

[Vitaly Davidovich]

The boolean seems better from an intent standpoint as well - all this wants
to do is tag a Class as having been internally generated by the proxy.
This information is present at the point of generation, obviously (and the
new. field can be marked final).  Doing the tagging via indirection
(ClassValue or otherwise) feels circuitous.  If the new boolean would
actually increase footprint, then OK - tagging via indirection to avoid
footprint increase for a relatively uncommon thing seems worthwhile.

I'm not that familiar with ClassValue but I don't doubt it scales better
than a synch map, and it may scale "well enough" for the expected usage.
But a read of a final field is obviously the best case.

Are there any downsides to using ClassValue? That is, what could go wrong?

Thanks

Sent from my phone
On Jan 24, 2013 10:46 AM, "Peter Levart" <peter.levart at gmail.com> wrote:

>  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();
>             &
> nbsp;   cache = new ConcurrentHashMap<Object,Reference<Class>>();
>                 loaderToCache.put(loader, cache);
>             }finally{
>               &
> nbsp; 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> 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
>>
>>
>