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