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

[Ioi Lam]

On Tue, 15 Sep 2020 18:57:55 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

Changes requested by iklam (Reviewer).

src/hotspot/share/classfile/lambdaFormInvokers.cpp line 51:

> 49: #include "runtime/os.hpp"
> 50: #include "runtime/signature.hpp"
> 51:

Are all these header files needed? E.g., typeArrayKlass.hpp doesn't seem to be needed.

src/hotspot/share/classfile/lambdaFormInvokers.cpp line 121:

> 119:     log_info(cds)("Class %s not present, skip", name);
> 120:     return;
> 121:   }

Since the classlist can be generated by the user, it may cause the assert at line 115 to fail. For example, no
java.lang.invoke.*$Holder classes are used by HelloWorld: $ java -verbose -Xshare:off -cp . HelloWorld | grep Holder
[0.030s][info][class,load] java.util.KeyValueHolder source: jrt:/java.base
[0.080s][info][class,load] java.security.SecureClassLoader$DebugHolder source: jrt:/java.base
$
But it's possible for the user to generate a classlist using HelloWorld, and then manually add LF_RESOLVE lines into
the classlist.

So I think line 114 should be changed to a regular lookup (the symbol is created if it doesn't exist), and line 115
should be removed.

Also, we should add some test cases for invalid LF_RESOLVE input. You can see examples in
[appcds/customLoader/ClassListFormatA.java](https://github.com/openjdk/jdk/blob/d250f9e08c4a0c047cd3e619918c116f568e31d4/test/hotspot/jtreg/runtime/cds/appcds/customLoader/ClassListFormatA.java#L51).
Since the new tests aren't related to custom loader, we should probably move
[appcds/customLoader/ClassListFormatBase.java](https://github.com/openjdk/jdk/blob/d250f9e08c4a0c047cd3e619918c116f568e31d4/test/hotspot/jtreg/runtime/cds/appcds/customLoader/ClassListFormatBase.java#L30)
under appcds/, and add a new file like appcds/ClassListFormat.java

src/hotspot/share/classfile/lambdaFormInvokers.cpp line 158:

> 156:   // find_class assert on SystemDictionary_lock or safepoint
> 157:   MutexLocker lock(SystemDictionary_lock);
> 158:   InstanceKlass* old = SystemDictionary::find_class(class_name, cld);

There's no need to call `find_class` here, since it will return the same class as `klass` on line 117.

src/hotspot/share/classfile/lambdaFormInvokers.hpp line 27:

> 25: #ifndef SHARE_MEMORY_LAMBDAFORMINVOKERS_HPP
> 26: #define SHARE_MEMORY_LAMBDAFORMINVOKERS_HPP
> 27: #include "memory/allocation.hpp"

For the AllStatic base class, you should use memory/allStatic.hpp instead.

src/hotspot/share/classfile/systemDictionary.cpp line 1875:

> 1873:             VerifyDuringStartup ||
> 1874:             VerifyAfterGC       ||
> 1875:             DumpSharedSpaces, "too expensive");

This may not be needed if you remove the find_class() call from LambdaFormInvokers::regenerate_holder_classes?

src/java.base/share/classes/java/lang/invoke/GenerateJLIClassesHelper.java line 67:

> 65:             if (VM.isDumpLoadedClassListSetAndOpen) {
> 66:                 VM.cdsTraceResolve(traceLF);
> 67:             }

GenerateJLIClassesHelper shouldn't need to know why the trace is needed. Also, "cdsTraceResolve" is too generic.

I think it's better to have
if (TRACE_RESOLVE || VM.CDS_TRACE_JLINV_RESOLVE) {
    ...
    VM.cdsTraceJLINVResolve(traceLF);

The acronym JLINV is used in
[methodHandles.cpp](https://github.com/openjdk/jdk/blob/ce93cbce77e1f4baa52676826c8ae27d474360b6/src/hotspot/share/prims/methodHandles.cpp#L1524)

src/java.base/share/classes/jdk/internal/misc/VM.java line 490:

> 488:      */
> 489:     public static boolean isDumpLoadedClassListSetAndOpen;
> 490:     private static native boolean isDumpLoadedClassListSetAndOpen0();

I would suggest to rename to `isDumpingLoadedClassList`

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

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