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

[Calvin Cheung]

Hi Ioi,

Thanks for taking another look.

I think I've made all the changes you suggested in the following updated 
webrevs:

inc: http://cr.openjdk.java.net/~ccheung/jdk15/8198698/webrev_delta.03/

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

Just one comment below.

On 6/7/20 10:59 PM, Ioi Lam wrote:
> Hi Calvin,
>
> Comments on the latest version 
> http://cr.openjdk.java.net/~ccheung/jdk15/8198698/webrev.02/
>
> ===============
>
> SystemDictionary::load_shared_class()
>
>   if (!SystemDictionaryShared::is_hidden_lambda_proxy(ik)) {
>     new_ik = KlassFactory::check_shared_class_file_load_hook(
>       ik, class_name, class_loader, protection_domain, cfs, CHECK_NULL);
>   }
>
> Do you know if CFLH is called for Lambda proxy classes when CDS is not 
> enabled? If so, we will be skipping the CFLH for the archived lambda 
> proxies.

CFLH check is skipped for VM hidden and anonymous classes. Below is from 
KlassFactory::create_from_stream:

   // Skip this processing for VM hidden or anonymous classes
   if (!cl_info.is_hidden() && (cl_info.unsafe_anonymous_host() == NULL)) {
     stream = check_class_file_load_hook(stream,
                                         name,
                                         loader_data,
                                         cl_info.protection_domain(),
                                         &cached_class_file,
                                         CHECK_NULL);
   }

I've added a comment to the code you listed above.

thanks,

Calvin

>
> If this is the case, I think for simplicity, we can disable the 
> archived lambda proxies when CFLH is enabled. CFLH is slow enough that 
> the optimization of lambda proxies will probably become noise.
>
> ===============
> Small nits:
>
> DTVerifierConstraint::_is_archived_lambda_proxy can be placed 
> immediately after _failed_verification to save space.
>
> DumpTimeLambdaProxyClassInfo::_proxy_klass -> should be renamed to 
> _proxy_klasses since it's an array that can contain more than one 
> proxy class.
>
> Similarly, RunTimeLambdaProxyClassInfo::_proxy_klass -> 
> _proxy_klass_head, since this is a linked list.
>
> add_to_dump_time_lambda_proxy_class_dictionary: -> should 
> assert(DumpTimeTable_lock->owned_by_self()) to make it clear that the 
> operations done in this function are thread-safe.
>
> ================
> 583:         ArchivePtrMarker::mark_pointer(&n_h);
>
> This call is not necessary because n_h is a pointer in the C stack. We 
> need to mark only the pointers that are in the CDS archive regions.
>
> ===============
> bool 
> SystemDictionaryShared::is_in_shared_lambda_proxy_table(InstanceKlass* 
> ik) {
>   assert(!DumpSharedSpaces && UseSharedSpaces, "called at run time 
> with CDS enabled only");
>   RunTimeSharedClassInfo* record = RunTimeSharedClassInfo::get_for(ik);
>   if (record != NULL && record->nest_host() != NULL) { <<<<<< HERE
>     return true;
>   } else {
>     return false;
>   }
> }
>
> This function will always return true if ik->is_hidden(), and will 
> assert if ik is not hidden:
>
>   InstanceKlass** nest_host_addr() {
>     assert(_klass->is_hidden(), "sanity"); <<<<< ASSERT
>     return (InstanceKlass**)(address(this) + nest_host_offset());
>   }
>   InstanceKlass* nest_host() {
>     return *nest_host_addr();
>   }
>
> If you want a strong assertion, we should use 
> _lambda_proxy_class_dictionary->iterate() to go over all the entries 
> and check that ik is in there. However, this table is modified when 
> proxies are loaded 
> (SystemDictionaryShared::get_shared_lambda_proxy_class), so we can't 
> see proxy classes that have already been loaded.
>
> For simplicity, I think we should just remove the following assert, 
> since there's no way for other types of hidden classes to be archived.
>
>     assert(is_in_shared_lambda_proxy_table(ik), "we don't archive 
> other hidden classes");
>
> ==========
>
> bool 
> SystemDictionaryShared::is_registered_lambda_proxy_class(InstanceKlass* 
> ik) {
>   DumpTimeSharedClassInfo* info = _dumptime_table->get(ik);
>   return (info != NULL) ? info->_is_archived_lambda_proxy && 
> !ik->is_non_strong_hidden() : false;
> }
>
> I think it's better to remove the "&& !ik->is_non_strong_hidden()" but 
> instead change the initialization of _is_archived_lambda_proxy to this:
>
>   if (info != NULL && !ik->is_non_strong_hidden()) {
>     // Set _is_archived_lambda_proxy in DumpTimeSharedClassInfo so 
> that the lambda_ik
>     // won't be excluded during dumping of shared archive. See 
> ExcludeDumpTimeSharedClasses.
>     info->_is_archived_lambda_proxy = true;
>
>     LambdaProxyClassKey key(caller_ik,
>                             invoked_name,
>                             invoked_type,
>                             method_type,
>                             member_method,
>                             instantiated_method_type);
>     add_to_dump_time_lambda_proxy_class_dictionary(key, lambda_ik);
>   }
>
> =======
> Some test cases need to update copyright year.
> ========
>
> The rest of the changes look good to me.
>
> Thanks
> - Ioi
>
> On 6/4/20 6:48 PM, Calvin Cheung wrote:
>> Hi Mandy,
>>
>> Thanks for taking another look.
>>
>> On 6/3/20 2:07 PM, Mandy Chung wrote:
>>>
>>>
>>> On 6/3/20 12:34 PM, Calvin Cheung wrote:
>>>>
>>>> I saw David has commented on this. So I'll leave the assert as 
>>>> before and I've added another assert (see line 1691):
>>>>
>>>> 1687   // The following ensures that the caller's nest host is the 
>>>> same as the lambda proxy's
>>>> 1688   // nest host recorded at dump time.
>>>> 1689   assert(nest_host->has_nest_member(caller_ik, THREAD) ||
>>>> 1690          nest_host == caller_ik, "caller_ik failed nest member 
>>>> check");
>>>>
>>>
>>> I don't think this assert is needed.  caller_ik can be a hidden 
>>> class and so this assert is not correct then.
>> I've removed it.
>>>
>>> Is there any issue to archive lambda proxy class whose caller is a 
>>> hidden class?  Is there any assumption in the CDS implementation 
>>> that the caller class is always a normal class?
>>
>> I've added a check in JVM_RegisterLambdaProxyClassForArchiving. If 
>> the caller class is hidden or vm anonymous, it will return.
>>
>> I also added 2 test cases to test the above. If the caller class is a 
>> hidden class, the test makes sure the corresponding lambda proxy 
>> class is not being archived. Currently, it doesn't seem possible to 
>> have a vm anonymous class to be the caller class of a lambda proxy 
>> class. I've added a test anyway so if things change later, we'll 
>> notice it.
>>
>>>
>>>> 1691   assert(nest_host == shared_nest_host, "mismatched nest host");
>>>>
>>>
>>> This is good.
>>>
>>>>
>>>> In SystemDictionary::load_shared_lambda_proxy_class, it checks the 
>>>> flag:
>>>>
>>>> 1422   if (initialize) {
>>>> 1423     loaded_ik->initialize(CHECK_NULL);
>>>> 1424   }
>>>>
>>>
>>>
>>> I think JVM_LookupLambdaProxyClassFromArchive is a more appropriate 
>>> place to link and initialize the class before return.   I expect 
>>> load_shared_lambda_proxy_class does loading only and linking and 
>>> initialization should be separate from loading.
>> Instead of putting the post loading code in the 
>> JVM_LookupLambdaProxyClassFromArchive function which would require 
>> changing some of the functions from private to public, I've renamed 
>> SystemDictionaryShared::load_shared_lambda_proxy_class to 
>> SystemDictionaryShared::prepare_shared_lambda_proxy class and moved 
>> the code there.
>>>
>>>> On a related note, in the existing jvm_lookup_define_class in jvm.cpp:
>>>>
>>>>   if (init) {
>>>>     ik->initialize(CHECK_NULL);
>>>>   } else {
>>>>     ik->link_class(CHECK_NULL);
>>>>   }
>>>>
>>>> I don't think the else is necessary as the 
>>>> ik->link_class(CHECK_NULL) has been done within the 
>>>> SystemDictionary::parse_stream.
>>>>
>>>
>>> Harold and Lois can chime in here.  I think ik->link_class may be 
>>> for unsafe anonymous class to prepare for constant pool patching.
>>>
>>>> Currently, the strong vs weak hidden class isn't recorded in the 
>>>> archive.
>>>>
>>>> :
>>>>
>>>> -----
>>>>
>>>> For now, I've added an assert in 
>>>> JVM_RegisterLambdaProxyClassForArchiving to make sure the hidden 
>>>> class is strong so that when things changed later, we'll notice it.
>>>>
>>>
>>> An assert is good.
>>>
>>>
>>> 3752   if (invokedName == NULL || invokedType == NULL || methodType 
>>> == NULL ||
>>> 3753       implMethodMember == NULL || instantiatedMethodType == 
>>> NULL) {
>>> 3754     return NULL;
>>> 3755   }
>>>
>>>
>>> Should this throw NPE instead?
>> I've made the change.
>>
>> Updated webrevs:
>>
>> inc: http://cr.openjdk.java.net/~ccheung/jdk15/8198698/webrev_delta.02/
>>
>> full: http://cr.openjdk.java.net/~ccheung/jdk15/8198698/webrev.02/
>>
>> thanks,
>>
>> Calvin
>>
>