RFR(XL) 8198698: Support Lambda proxy classes in dynamic CDS archive
Calvin Cheung
calvin.cheung at oracle.com
Tue Jun 9 21:47:02 UTC 2020
Hi David,
On 6/9/20 12:50 AM, David Holmes wrote:
> Hi Calvin,
>
> Just for completeness I took a look at the nest host parts and that
> all seems fine to me now. It's not a full review by any means.
Thanks for reviewing the nest host part.
>
> One style nit I noticed:
>
> src/hotspot/share/classfile/systemDictionaryShared.cpp
>
> + InstanceKlass* _caller_ik;
> + Symbol* _invoked_name;
> + Symbol* _invoked_type;
> + Symbol* _method_type;
> + Method* _member_method;
> + Symbol* _instantiated_method_type;
>
> You can line them up if you like (not essential though) but they
> shouldn't be spaced so far apart :)
I've removed the extra spaces.
thanks,
Calvin
>
> Thanks,
> David
> -----
>
> On 9/06/2020 10:34 am, 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