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
