On Tue, 6 Oct 2020 18:12:50 GMT, Mandy Chung <mch...@openjdk.org> wrote:
>> Yumin Qi has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Removed unused imports.
>
> 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.
Done
> 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.
Done
> 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.
Done
-------------
PR: https://git.openjdk.java.net/jdk/pull/193
