RFR: 8276184: Exclude lambda proxy class from the CDS archive if its caller class is excluded [v3]

[Calvin Cheung]

On Fri, 5 Nov 2021 17:09:57 GMT, Ioi Lam <iklam at openjdk.org> wrote:

> Looks good. I have some comments about a potential bug and suggestion for more testing.
> 
> Also, SystemDictionaryShared::cleanup_lambda_proxy_class_dictionary() is called separately for static and dynamic dumps. I think we should move this function to the end of SystemDictionaryShared::check_excluded_classes().

Fixed. Also changed the declaration of `SystemDictionaryShared::cleanup_lambda_proxy_class_dictionary()` to private.

> src/hotspot/share/classfile/systemDictionaryShared.cpp line 1617:
> 
>> 1615:     InstanceKlass* caller_ik = key.caller_ik();
>> 1616:     if (SystemDictionaryShared::check_for_exclusion_impl(caller_ik, true)) {
>> 1617:       // If the caller class is excluded, unregister all the associated lambda proxy classes so that
> 
> As we discussed offline, I think we should add a comment here that SystemDictionaryShared::is_excluded_class() cannot be called because with dynamic dumps, caller_ik could be a class in the base archive.
> 
> Also, I think we should test different combinations of how a proxy class can be excluded by the two conditions in this function:
> 
> (1) caller_ik is excluded
> (2) proxy itself is excluded (because it implements an interface with an old class version
> (3) both (1) and (2) are true

Added a comment.
The RedefineCallerClassTest.java has been expanded to cover the above 3 cases.

> src/hotspot/share/classfile/systemDictionaryShared.cpp line 1626:
> 
>> 1624:     for (int i = 0; i < info._proxy_klasses->length(); i++) {
>> 1625:       InstanceKlass* ik = info._proxy_klasses->at(i);
>> 1626:       if (SystemDictionaryShared::check_for_exclusion_impl(ik, true)) {
> 
> With the original code, if the list has two elements, only the first one is removed -- after the first iteration, `i` becomes 1, but `_proxy_klasses->length()` is also 1, and thus the loop will terminate.
> 
> I think the loop should be reversed:
> 
> for (int i = info._proxy_klasses->length() - 1; i >= 0; i--) {
>   if (...) {
>     info._proxy_klasses->remove_at(i);
>   }
> }
> 
> 
> For test coverage, we should also have two test case:
> 
> - caller class Foo1 that uses a single instance of an lambda based on old interface
> - caller class Foo2 that uses two instances of an lambda based on old interface

Nice catch! I've fixed the code.
I've expanded the existing LambdaContainsOldInf.java test to cover the above cases.

> src/hotspot/share/classfile/systemDictionaryShared.hpp line 190:
> 
>> 188: 
>> 189: public:
>> 190:   static bool check_for_exclusion_impl(InstanceKlass* k, bool silent);
> 
> Since check_for_exclusion_impl is used only by CleanupDumpTimeLambdaProxyClassTable, I think it's better to keep it private, but declare CleanupDumpTimeLambdaProxyClassTable as a friend class of SystemDictionaryShared. We already do that for ExcludeDumpTimeSharedClasses.

Fixed.

> test/hotspot/jtreg/runtime/cds/appcds/dynamicArchive/RedefineCallerClassTest.java line 29:
> 
>> 27:  * @summary If the caller class is redefined during dump time, the caller class
>> 28:  *          and its lambda proxy class should not be archived.
>> 29:  * @requires vm.cds
> 
> Add `@bug .....`

Fixed.

> test/hotspot/jtreg/runtime/cds/appcds/test-classes/Hello.java line 28:
> 
>> 26:   public static void main(String args[]) {
>> 27:     System.out.println("Hello World");
>> 28:     System.out.println(getRunnable());
> 
> Since this class is used by many test cases, I think it's better to control its behavior by
> 
> 
> if (args.length > 1 && args[0].equals("testlambda")) {
>   System.out.println(getRunnable());
> }
> 
> 
> And also add `@bug JDK-8276184` and a note in SignJar.java, something like "testlambda is for testing JDK-8276184".

Fixed.

-------------

PR: https://git.openjdk.java.net/jdk/pull/6205