On Wed, 16 Sep 2020 19:05:56 GMT, Ioi Lam <ik...@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).
> _Mailing list message from [Mandy Chung](mailto:mandy.ch...@oracle.com) on > [build-dev](mailto:build-dev@openjdk.java.net):_ > src/java.base/share/classes/java/lang/invoke/GenerateJLIClassesHelper.java > 367 /** 368 * called from vm to generate MethodHandle holder classes 369 > * @return @code { Object[] } if holder classes can be generated. 370 * > @param lines the output lines from @code { VM.cdsTraceResolve } 371 */ > @code {....} is wrong syntax. It should be {@code ....} 372 static > Object[] cdsGenerateHolderClasses(String[] lines) { this can be made > private as it's invoked by VM only. Can you move it to the end of the Will change access to 'private' > file. 373 try { 374 Map<String, byte[]> result = > generateHolderClasses(Arrays.stream(lines)); 375 if (result == null) { > 376 return null; 377 } > > generateHolderClasses should never return null and so line 371-377 can > be dropped. ? I also suggest to add in the generateHolderClasses method > to add Objects.requireNonNull(traces). > Will drop the check and add Objects.requireNonNull(traces). > 379???????????? Object[] ret_array = new Object[size * 2]; > > Rename `ret_array` to retArray to follow the naming convention using camel > case. > > src/java.base/share/classes/jdk/internal/misc/VM.java > 457 isDumpLoadedClassListSetAndOpen = isDumpLoadedClassListSetAndOpen0(); > > This should be cached in the caller who checks if -XX:+DumpLoadedClassList > instead of every VM initialization. > > Since there are many CDS-related methods, I think it's time to introduce > a new CDS class for these methods to reside (maybe jdk.internal.vm.CDS?). > > I suggest to add CDS:logTraceResolve(String line) method that > will check if isDumpLoadedClassListSetAndOpen is true, then call the > native cdsTraceResolve (or whatever name). This way, > GenerateJLIClassesHelper can simply call CDS::logTraceResolve. > Created a separate issue https://bugs.openjdk.java.net/browse/JDK-8253208 to move CDS method to CDS.java All CDS related code will be added to CDS.java > 493 * Output to DumpLoadedClassList, format is simimar to LF_RESOLVE > > s/simimar/similar > > 494 * @see InvokerBytecodeGenerator > 495 * @param line the line to output. > > @see is typically placed after @param. > > Should it say @see java.lang.invoke.GenerateJLIClassesHelper::traceLambdaForm > instead of InvokerBytecodeGenerator? > Right, will change that to the suggestion. > Mandy > > On 9/15/20 12:15 PM, Yumin Qi wrote: ------------- PR: https://git.openjdk.java.net/jdk/pull/193