RFR: 8247536: Support for pre-generated java.lang.invoke classes in CDS static archive [v12]

[Yumin Qi]

On Wed, 7 Oct 2020 20:36:18 GMT, Mandy Chung <mchung at openjdk.org> wrote:

>> Yumin Qi has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains
>> 23 commits:
>>  - Added new separate function to CDS for logging species and modified the existing function to log lambda form invokers.
>>    Changed isDumpLoadedClassList to a reasonable name isDumpingClassList as read only in CDS.
>>  - Merge branch 'master' of https://github.com/openjdk/jdk into jdk-8247536
>>  - Removed unused imports.
>>  - Fixed comments with correct class and method name in CDS, removed unused variables after last change.
>>  - Moved and renamed cdsGenerateHolderClasses from GenerateJLIClassesHelp to CDS as generateLambdaFormHolderClasses. Added
>>    input verification function in CDS before class generation. Added more test scenarios. Removed trailing unused ending
>>    words for output of lambda form trace line in case of DumpLoadedClassList.
>>  - Move the check work to java, restore code in VM. Modified test code according to the changes. The invoke name
>>    verififcation is not implemented since not all the holder class are processed, not all the functions of processed
>>    holder classes are added. For holder class with DirectMethodHandle in its name, only the name in the
>>    DMH_METHOD_TYPE_MAP keyset is added, ithe line with other names just gets skipped silently. This makes the verification
>>    on invoke names difficul, a name not in the keyset should not fail the test. Also add a boolean to
>>    cdsGenerateHolderClasses to indicate call path.
>>  - Remove trailing word of line which is not used in holder class regeneration. There is a trailing LF (Line Feed) so trim
>>    white spaces from both front and end of the line or it will fail method type validation.
>>  - In case of exception happens during reloading class, CHECK will return without free the allocated buffer for class
>>    bytes so moved the buffer allocation and freeing to caller. Also removed test 6 since there is not guarantee that we
>>    can give a signature which will always fail. Additional changes to GenerateJLIClassesHelper according to review
>>    suggestion.
>>  - Merge branch 'master' of https://github.com/openjdk/jdk into jdk-8247536
>>  - Merge branch 'master' of https://git.openjdk.java.net/jdk into jdk-8247536
>>  - ... and 13 more: https://git.openjdk.java.net/jdk/compare/82fe023b...f5584dcf
>
> src/java.base/share/classes/jdk/internal/misc/CDS.java line 144:
> 
>> 142:             String line = s.trim();
>> 143:             if (!line.startsWith("[LF_RESOLVE]") && !line.startsWith("[SPECIES_RESOLVE]")) {
>> 144:                 System.out.println("Wrong prefix: " + line);
> 
> Should this throw an exception instead?

This part is for check the format only, throw exceptions will lead more objects generated which should not be archived
in shared heap. Since this is only called from VM, so decide not to throw exception here.

> src/java.base/share/classes/jdk/internal/misc/CDS.java line 40:
> 
>> 38:       * indicator for dumping class list.
>> 39:       */
>> 40:     static public final boolean isDumpingClassList;
> 
> what about making this a private static field and adding a public static `isDumpingClassList()` method (which was in
> the previous version).

That will have a name for two properties, if you that is OK, I will use the previous version.

> src/java.base/share/classes/jdk/internal/misc/CDS.java line 155:
> 
>> 153:                     System.out.println("Incorrecct number of items in the line: " + parts.length);
>> 154:                     System.out.println("line: " + line);
>> 155:                     return null;
> 
> I think these error cases should throw `IllegalArgumentException` and VM decides how to handle the exception.

Same reason as above.

> src/java.base/share/classes/jdk/internal/misc/CDS.java line 140:
> 
>> 138:     // return null for invalid input
>> 139:     private static Stream<String>  validateInputLines(String[] lines) {
>> 140:         ArrayList<String> list = new ArrayList<String>(lines.length);
> 
> Nit: this can use diamond operatior like this: `new ArrayList<>(lines.length)`.

Will update.

> src/java.base/share/classes/jdk/internal/misc/CDS.java line 184:
> 
>> 182:         Objects.requireNonNull(lines);
>> 183:         try {
>> 184:             Stream<String> lineStream = validateInputLines(lines);
> 
> It seems clearer to have `validateInputLines` do validation only and convert this line into:
> 
>   validateInputLines(lines);
>   Stream<String> lineStream = Arrays.stream(lines);

Somewhere in the testing framework, the line in ExtraClassList was added '\f' which needs trim, I did not dig into deep
the  root cause so here just return a new List with ending white spaces trimmed instead. I will file a bug for it and
when it fixed, we can do this way.

> src/java.base/share/classes/jdk/internal/misc/CDS.java line 178:
> 
>> 176:     /**
>> 177:      * called from vm to generate MethodHandle holder classes
>> 178:      * @return @code { Object[] } if holder classes can be generated.
> 
> type: `s/@code { Object[]/{@code Object[]}`

Will update

> src/java.base/share/classes/jdk/internal/misc/CDS.java line 198:
> 
>> 196:             return retArray;
>> 197:         } catch (Exception e) {
>> 198:             e.printStackTrace();
> 
> Is this a debugging statement?   If CDS swallows the exception thrown, I think VM should emit the warning message and
> print the stack trace if appropriate.

I want to print this in java since in VM it is a little complex than java. It just used to give the stack trace where
the exception happened for debug purpose. Maybe I should put comment for it.

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

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