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

Ioi Lam ioi.lam at oracle.com
Mon Jun 8 21:58:46 UTC 2020


Hi Calvin,

Looks good. Just some minor nits:

I think the following condition:

  if (info != NULL && !lambda_ik->is_non_strong_hidden()) {

should also apply to the 
add_to_dump_time_lambda_proxy_class_dictionary() call. That way, you 
won't have an unexpected entry in the dump time proxy dictionary.

Also, is_in_shared_lambda_proxy_table() can be removed since it's no 
longer used.

Thanks
- Ioi



On 6/8/20 1:56 PM, Calvin Cheung wrote:
> 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
>>>
>>



More information about the hotspot-runtime-dev mailing list