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

[Mandy Chung]

On Tue, 6 Oct 2020 20:46:17 GMT, Yumin Qi <minqi at openjdk.org> wrote:

>> This patch is reorganized after 8252725, which is separated from this patch to refactor jlink glugin code. The previous
>> webrev with hg can be found at: http://cr.openjdk.java.net/~minqi/2020/8247536/webrev-05. With 8252725 integrated, the
>> regeneration of holder classes is simply to call the new added GenerateJLIClassesHelper.cdsGenerateHolderClasses
>> function.  Tests: tier1-4
>
> 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 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).

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?

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.

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)`.

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);

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[]}`

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.

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

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