RFR(XL) 8198698: Support Lambda proxy classes in dynamic CDS archive
Ioi Lam
ioi.lam at oracle.com
Tue Jun 9 00:52:06 UTC 2020
Hi Calvin, These look good to me. Thanks!
- Ioi
On 6/8/20 5:34 PM, Calvin Cheung wrote:
>
> 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