RFR(XL) 8198698: Support Lambda proxy classes in dynamic CDS archive

[David Holmes]

Hi Mandy,

On 3/06/2020 4:38 am, Mandy Chung wrote:
> Hi Calvin,
> 
> To recap an offline discussion, I think we should use JDK bootcycle 
> build to test this feature.  For example generating a boot JDK with 
> archived lambda proxy classes for java.base and javac to build itself 
> and also use it to run JDK tier 1-3 tests.  This would serve a good test 
> bed.   I also suggest to experiment how JDK can benefit from this 
> feature (e.g.  any java.base and javac performance gain). The module 
> system initialization had to avoid using lambdas at startup time and we 
> could re-examine this again.

I don't see how this affects system initialization given it requires use 
of CDS which is optional.

> On 5/29/20 2:29 PM, Calvin Cheung wrote:
>> RFE: https://bugs.openjdk.java.net/browse/JDK-8198698
>>
>> webrev: http://cr.openjdk.java.net/~ccheung/jdk15/8198698/webrev.00/
>>
> 
> Hidden classes have the following properties:
> 1. not discoverable by name.  It's not registered in the system dictionary.
> 2. It can be a dynamic nest member of a nest host.  For lambda proxy 
> class, it has the same nest host as the caller class (same runtime 
> package as the caller class)
> 3. strong or weak link with the class loader.  a hidden class may be 
> unloaded even its defining class loader is alive.
> 
> For the archived lambda proxy classes, do you re-assign a new class name 
> to the shared lambda proxy class?  I suspect not.  It's very rare that 
> it will conflict with the name of other hidden classes. It's an option 
> to patch the name to indicate it's from shared archive if it helps 
> diagnosability.
> 
> My comments below are related to the above.
> 
> 1374 InstanceKlass* 
> SystemDictionary::load_shared_lambda_proxy_class(InstanceKlass* ik,
> 1375 Handle class_loader,
> 1376 Handle protection_domain,
> 1377 PackageEntry* pkg_entry,
> 1378 TRAPS) {
> 1379 InstanceKlass* shared_n_h = 
> SystemDictionaryShared::get_shared_nest_host(ik);
> 1380 assert(shared_n_h->is_shared(), "nest host must be in CDS archive");
> 1381 Symbol* cn = shared_n_h->name();
> 1382 Klass *s = resolve_or_fail(cn, class_loader, protection_domain, 
> true, CHECK_NULL);
> 1383 if (s != shared_n_h) {
> 1384 // The dynamically resolved nest_host is not the same as the one we 
> used during dump time,
> 1385 // so we cannot use ik.
> 1386 return NULL;The above resolve_or_fail is to ensure that H is the 
> same class loaded by the given class loader.  Another validation that 
> you should do is to check
> the nest host and nest member must be in the same runtime package -
> maybeik->set_nest_host(shared_n_h, THREAD) validates the runtime package??

Aren't we assuming/requiring that the dump is valid in that regard? 
Otherwise there would be many checks that would have to be re-done to 
ensure an archived class matches the "same" class that would be 
dynamically loaded. ??

> 
> SystemDictionaryShared::load_shared_lambda_proxy_class
> 
> 1679   InstanceKlass* shared_n_h = get_shared_nest_host(lambda_ik);
> 1680   assert(shared_n_h != NULL, "unexpected NULL _nest_host");
> 1681
> 1682   InstanceKlass* loaded_lambda =
> 1683     SystemDictionary::load_shared_lambda_proxy_class(lambda_ik, 
> class_loader, protection_domain, pkg_entry, CHECK_NULL);
> 1684
> 1685   InstanceKlass* n_h = caller_ik->nest_host(THREAD);
> 1686   assert(n_h->has_nest_member(caller_ik, THREAD) ||
> 1687          n_h == caller_ik, "mismatched nest host");
> 
> I think you meant to check n_h->has_nest_member(loaded_lambda, THREAD).

That can't be right. has_nest_member is for checking static nest 
membership. Dynamic proxies are not static nest members!

David
-----

> It'd be good to add a comment to say that this ensures thatthe caller's 
> nest host must be the same as the lambda proxy's nest host recorded at 
> dump time.
> Nit: the variable name "n_h" or "_n_h" would be clearer if it's named
> "nest_host" or "_nest_host".
> 
> 1689   SystemDictionary::set_shared_class_init_state(lambda_ik, 
> InstanceKlass::loaded);
> 
> A lambda proxy class typically is eagerly initialized (unless a flag is 
> given).
> Your change dumps the hidden class with no knowledge if 
> `Lookup::defineHiddenClass`
> is called with "initialize" parameter is true or not.  This may affect 
> the runtime
> performance (for example MethodHandle will have class init barrier if 
> the class
> is not yet initialized).
> This may have performance implication. I think we should consider 
> passing the initialize parameter to LambdaProxyClassArchive::find such 
> that if the class is found from CDS archive, it will invoke <clinit> 
> before it returns to the library. One other point I brought up - strong 
> vs weak hidden class (i.e. is it using the primary CLD or the per-mirror 
> CLD?). I have lost myself where you do this in the implementation. Can 
> you point that out? Is it recorded in the shared archive and make sure 
> it uses the exact CLD when it's loaded?
> 
> JVM_RegisterLambdaProxyClassForArchiving - we can add an assert to check 
> if lambdaProxyClass is a hidden class. JVM_IsCDSDumpingEnabled and 
> JVM_IsCDSSharingEnabled - an alternative library/VM interface is to use 
> internal system properties that jdk.internal.misc.VM::saveProperties 
> reads and removes these flags. InnerClassLambdaMetafactory 247 /** Use 
> the LambdaProxyClassArchive for including the lambda proxy class 248 * 
> in CDS archive and retrieving the class from the archive. If the class 
> 249 * is not found in the archive, it calls generatesInnerClass to 
> create 250 * the class. 251 */ What about simpler comment: /* * Spins 
> the lambda proxy class. * * This first checks if a lambda proxy class 
> can be loaded from CDS archive. * Otherwise, generate the lambda proxy 
> class. */ This adds a VM call for every lambda proxy class creation. Do 
> you have any the performance measurement when CDS is not on? Any 
> performance regression? Mandy
>