RFR(XL) 8198698: Support Lambda proxy classes in dynamic CDS archive
Mandy Chung
mandy.chung at oracle.com
Tue Jun 9 16:25:51 UTC 2020
The new version looks okay.
Nit: systemDictionaryShared.cpp line 1700-1701 have 4-space indentation
which should be fixed.
I mentioned previously that a hidden class may have live class data
attached to it when it's created. This will be broken if LMF were
changed to use class data. You can add an assert in
add_lambda_proxy_class that the class being archived must have null
class data (see java_lang_Class::class_data).
No need for a new webrev.
thanks
Mandy
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