RFR(XL) 8198698: Support Lambda proxy classes in dynamic CDS archive
Calvin Cheung
calvin.cheung at oracle.com
Tue Jun 9 00:34:00 UTC 2020
On 6/8/20 2:58 PM, Ioi Lam wrote:
> 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.
The following updated incremental webrev should cover the above items as
well as another item we discussed off-list: adding the
lambda_ik->set_shared_classpath_index so that some checks for classpath
index in systemDictionary.cpp can be removed.
http://cr.openjdk.java.net/~ccheung/jdk15/8198698/webrev_delta.04/
thanks,
Calvin
>
> 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