[8] Review request for 8008770: SerializedLambda incorrect class loader for lambda deserializing class

[Peter Levart]

Hi,

I experimented with caching of CallSites by the LambdaMetafactory. 
Here's what I came up with:

http://dl.dropbox.com/u/101777488/jdk8-lambda/InnerClassLambdaMetafactory/webrev.01/index.html

This implementation is very simple, but effective. Besides serializable 
lambda being always deserialized into same class, the presented caching 
also optimizes method references: each distinct (method reference, SAM 
target type) pair uses a single proxy class per capturing class. Without 
caching each method reference expression uses a distinct proxy class.

What do you think? Am I correct in asserting that all types appearing in 
LambdaMetafactory arguments are always visible from the capturing class 
and therefore there is no problem referencing the j.l.Class objects of 
those types from the capturing class' j.l.Class object as far as class 
memory leaks are concerned? If I'm mistaken, the caching key could be 
modified to retain only symbolic names of types instead of j.l.Class 
objects and still be unique, since no two distinct types with same 
symbolic name can be visible from the same class.

Regards, Peter

On 03/05/2013 12:03 AM, Peter Levart wrote:
> If there is a 1-to-1 mapping between a lambda expression and a 
> synthetic implementation method generated by javac and further if 
> there could be a 1-to-1 mapping between the synthetic implementation 
> method and the SAM proxy class name (calculated in advance) then the 
> class-loader of the capturing class itself could be used as a cache to 
> look-up (findLoadedClass) if the proxy class has already been defined 
> or if it has to be defined atomically. Once we have a single Class 
> object per lambda expression/implementation method, ClassValue can be 
> used to cache the CallSite object on it.
>
> I experimented with the following caching scheme which returns a singe 
> CallSite instance for both capturing sites (lambda expression and 
> $deserializeLambda$):
>
> public static CallSite altMetaFactory(MethodHandles.Lookup caller,
>                                           String invokedName,
>                                           MethodType invokedType,
>                                           Object... args)
>             throws ReflectiveOperationException, 
> LambdaConversionException {
>         MethodHandle samMethod = (MethodHandle)args[0];
>         MethodHandle implMethod = (MethodHandle)args[1];
>         MethodType instantiatedMethodType = (MethodType)args[2];
>         int flags = (Integer) args[3];
>         Class[] markerInterfaces;
>         int argIndex = 4;
>         if ((flags & FLAG_MARKERS) != 0) {
>             int markerCount = (Integer) args[argIndex++];
>             markerInterfaces = new Class[markerCount];
>             System.arraycopy(args, argIndex, markerInterfaces, 0, 
> markerCount);
>             argIndex += markerCount;
>         }
>         else
>             markerInterfaces = EMPTY_CLASS_ARRAY;
>         AbstractValidatingLambdaMetafactory mf;
>         mf = new InnerClassLambdaMetafactory(caller, invokedType, 
> samMethod, implMethod, instantiatedMethodType,
>                                              flags, markerInterfaces);
>         mf.validateMetafactoryArgs();
>
>         MethodHandleInfo implInfo = new MethodHandleInfo(implMethod);
>         ConcurrentMap<String, CallSite> callSites = 
> CALL_SITES_CV.get(implInfo.getDeclaringClass());
>         CallSite callSite = callSites.get(implInfo.getName());
>         if (callSite == null) {
>             callSite = mf.buildCallSite();
>             CallSite oldCallSite = 
> callSites.putIfAbsent(implInfo.getName(), callSite);
>             if (oldCallSite != null)
>                 callSite = oldCallSite;
>         }
>
>         return callSite;
>     }
>
>     // each implementation method's declaring class has a Map of 
> CallSites keyed by implementation method name
>
>     private static final ClassValue<ConcurrentMap<String, CallSite>> 
> CALL_SITES_CV =
>         new ClassValue<ConcurrentMap<String, CallSite>>() {
>             @Override
>             protected ConcurrentMap<String, CallSite> 
> computeValue(Class<?> type) {
>                 return new ConcurrentHashMap<>();
>             }
>         };
>
>
> ... and it does it's job for the price of ClassValue and 
> ConcurrentHashMap overhead even if only one call to metafactory is 
> ever performed for each lambda expression.
>
> Using ClassLoader as a cache might be an opportunity to do it without 
> overhead.
>
>
> Regards, Peter
>
>
>
> On 03/04/2013 08:04 PM, Brian Goetz wrote:
>> Its a good problem to kick down the road.  When/if we switch to a
>> class-per-SAM instead of class-per-callsite implementation of lambda
>> conversion, the issue goes away.
>>
>> On 3/4/2013 1:38 PM, Remi Forax wrote:
>>> On 03/04/2013 03:26 PM, Brian Goetz wrote:
>>>> You are correct.  This is less than ideal, but allowable, and we're
>>>> treating this as a quality-of-implementation issue.  The solution would
>>>> be to "outline" both capture sites into a private method and replace
>>>> them both with a call to that method; then they would share the
>>>> invokedynamic call site.  It's on the list of "possible future
>>>> optimizations."
>>> One solution is, if the lambda is serializable, to emulate invokedynamic
>>> using constant method handles stored in static final fields, thus
>>> shareable .
>>> But this make the translation scheme for javac ugly.
>>>
>>> Another solution is to have an API to query already existing CallSite
>>> objects,
>>> so the invokedynamic in $deserializeLambda$ will effectively share the
>>> same lambda proxy.
>>> It's a good question for the JSR 292 EG :)
>>>
>>> Rémi
>>>
>>>> On 3/4/2013 3:18 AM, Peter Levart wrote:
>>>>> Hi Robert,
>>>>>
>>>>> I noticed that when the same VM is used both for evaluating a lambda
>>>>> expression (producing a SAM instance) and for de-serializing a
>>>>> previously serialized SAM instance representing the same lambda
>>>>> expression, two distinct SAM proxy classes are generated (and
>>>>> consequently, even non-capturing lambdas come out as two distinct
>>>>> instances). I believe this is because there are two places where
>>>>> LambdaMetafactory is called - the "indy" call generated by javac in the
>>>>> place of a lambda expression and one in the "$deserializeLambda$" method
>>>>> of the capturing class - and each call produces a distinct lambda
>>>>> factory with a distinct generated proxy class.
>>>>>
>>>>> Do you feel that this situation is rare and/or that maintaining a cache
>>>>> of "CallSite" objects per "altMetaFactory" request parameters would
>>>>> actually present a greater overhead than generating two classes in such
>>>>> scenarios?
>>>>>
>>>>> Regards, Peter
>>>>>
>>>>> On 02/25/2013 06:24 PM, Robert Field wrote:
>>>>>> Please review the fixes for CRs:
>>>>>>
>>>>>>              http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=8008770
>>>>>>
>>>>>> Webrev:
>>>>>>
>>>>>>              http://cr.openjdk.java.net/~rfield/8008770_2/
>>>>>>
>>>>>> Thank,
>>>>>> Robert
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>