RFR(XL) 8198698: Support Lambda proxy classes in dynamic CDS archive

[Calvin Cheung]

Hi Ioi,

Thanks for your review.

On 6/1/20 11:17 AM, Ioi Lam wrote:
> Hi Calvin,
>
> Some initial feedback:
>
> jvm.cpp:
>
> JVM_ENTRY(void, JVM_RegisterLambdaProxyClassForArchiving(...,
>                                               jstring invokedName,
>                                               jobject invokedType...))
>
>   Symbol* invoked_name = NULL;
>   if (invokedName != NULL) {
>     invoked_name = 
> java_lang_String::as_symbol_or_null(JNIHandles::resolve_non_null(invokedName));
>   }
>   Handle invoked_type_oop(THREAD, 
> JNIHandles::resolve_non_null(invokedType));
>   Symbol* invoked_type = 
> java_lang_invoke_MethodType::as_signature(invoked_type_oop(), false);
>
>   ...
>   SystemDictionaryShared::add_lambda_proxy_class(caller_ik, lambda_ik, 
> invoked_name, invoked_type,
>                                                  method_type, m, 
> instantiated_method_type);
>
>
> Theoretically, it's possible for java_lang_String::as_symbol_or_null 
> and java_lang_invoke_MethodType::as_signature, etc, to return a NULL 
> if the symbol is not present in the SymbolTable. I am not sure if in 
> practice this may be true, but it seems safer to create the symbols if 
> they don't exist yet, because you are going to record them anyway. So 
> I would suggest using java_lang_String::as_symbol(), and passing 
> intern_if_not_found==true to the as_signature() methods. This will 
> have the side effect of adding 1 to the Symbol's reference count, 
> which is needed because you are recording them in 
> add_lambda_proxy_class().
>
>
> Also, in JVM_LookupLambdaProxyClassFromArchive, I think your existing 
> code in SystemDictionaryShared::get_shared_lambda_proxy_class should 
> be able to handle NULL values, but it would be safer to add
>
>    if (invoked_name == NULL || .... || instantiated_method_type == 
> NULL) {
>      return NULL;
>    }

I've made the above changes.

>
> instanceKlass.cpp:
>
> Could you explain why this is necessary?
>
> 2489 
> set_package(ClassLoaderData::class_loader_data_or_null(loader_data->class_loader()), 
> pkg_entry, CHECK);

I've reverted the change to the above line. Don't remember why it was 
changed.


Updated webrevs:

delta: http://cr.openjdk.java.net/~ccheung/jdk15/8198698/webrev_delta.01/

full: http://cr.openjdk.java.net/~ccheung/jdk15/8198698/webrev.01/

The webrevs include changes based on Mandy's comments.

I'll be making more changes based on her latest comments.

thanks,

Calvin

>
>
> (... more to come ...)
>
> Thanks
> - Ioi
>
>
>
> On 5/29/20 2:29 PM, Calvin Cheung wrote:
>> RFE: https://bugs.openjdk.java.net/browse/JDK-8198698
>>
>> webrev: http://cr.openjdk.java.net/~ccheung/jdk15/8198698/webrev.00/
>>
>> There's a high level design doc in the attachment[1] of the RFE.
>>
>> Passed tiers 1 - 4 tests (including the new tests).
>>
>> thanks,
>>
>> Calvin
>>
>> [1] 
>> https://bugs.openjdk.java.net/secure/attachment/88446/archive_lambda_proxy.txt
>>
>