RFR(XL) 8198698: Support Lambda proxy classes in dynamic CDS archive
Ioi Lam
ioi.lam at oracle.com
Mon Jun 8 21:58:46 UTC 2020
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.
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