On Tue, 15 Sep 2020 18:57:55 GMT, Yumin Qi <mi...@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