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

[Yumin Qi]

On Wed, 16 Sep 2020 19:05:56 GMT, Ioi Lam <iklam 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).

> _Mailing list message from [Mandy Chung](mailto:mandy.chung at oracle.com) on
> [build-dev](mailto:build-dev at 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