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

Ioi Lam ioi.lam at oracle.com
Mon Jun 8 05:59:09 UTC 2020


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.

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