On Tue, 6 Oct 2020 17:35:18 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
>
> Yumin Qi has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Removed unused imports.
src/java.base/share/classes/java/lang/invoke/GenerateJLIClassesHelper.java line
63:
> 61: if (TRACE_RESOLVE) {
> 62: System.out.println(traceLF + (resolvedMember != null ? "
> (success)" : " (fail)"));
> 63: }
I suggest not to change the existing code. Instead, have
`CDS::traceLambdaFormInvoker`
to take individual parameters `Class<?> holder, String name, String
shortenSignature`
(rather than the formatted string). Something like:
if (CDS.isDumpLoadedClassList()) {
CDS.traceLambdaFormInvoker(holder, name,
shortenSignature(basicTypeSignature(type));
}
This also gives flexibility to CDS to decide on what format to write to the
class list (like this case, you drop the
text "success/fail")
In addition, the conditional check on `CDS.isDumpLoadedClassList()` is hard to
relate to why CDS traces these events.
I see Ioi's comment on this method name too. I agree with Ioi that
`isDumpingClassList` makes more sense.
src/java.base/share/classes/java/lang/invoke/GenerateJLIClassesHelper.java line
74:
> 72: System.out.println(traceSP + (salvage != null ? "
> (salvaged)" : " (generated)"));
> 73: }
> 74: CDS.traceLambdaFormInvoker(traceSP);
I suggest leaving the existing code unchanged. Instead, add the following:
if (CDS.isDumpingClassList()) {
CDS.traceSpeciesType(cn);
}
The above uses Ioi's suggested method name which reads better.
src/java.base/share/classes/jdk/internal/misc/CDS.java line 83:
> 81: * check if -XX:+DumpLoadedClassList and given file is open
> 82: */
> 83: public static boolean isDumpLoadedClassList() {
I agree with Ioi's suggestion to rename this to `isDumpingClassList` which
describes what the VM is doing.
-------------
PR: https://git.openjdk.java.net/jdk/pull/193
