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

[Yumin Qi]

HI, Mandy

   Thanks for the review, I took one day off yesterday so just got a detail look of your reply.

On 8/19/20 1:30 PM, Mandy Chung wrote:
> On 8/17/20 12:37 PM, Yumin Qi wrote:
>> Hi, Ioi
>>
>>   Thanks for review/suggestion. I have updated the webrev at the following link:
>>
>> http://cr.openjdk.java.net/~minqi/2020/8247536/webrev-02/
>>
>
> This patch leverages the TRACE_RESOLVE output and passes the trace output to VM.  VM then calls GenerateJLIClassesHelper::generateMHHolderClasses to do the parsing and generate Holder class per the resolved LFs.   I think there are other cleaner alternatives implementing this.   jlink --generate-jli-classes plugin depends the trace output whereas -Xshare:dump does not.   It's cleaner to skip generating the trace output and parsing for dumping shared archive purpose.  In addition, the implementation needs some cleanup (I can send you feedback on the next revision)
>
Current patch did not change the existing code for JLinkPlugin part. I just moved the parsing code from GenerateJLIClassesPlugin.java to GenerateJLIClassesHelper.java since the former is an internal class to which we shouldn't call to generate holder classes.
> Instead of relying on a system property "java.lang.invoke.MethodHandle.CDS_TRACE_RESOLVE", it's better to use jdk.internal.vm.isCDSDumpingEnabled() to detect if this is CDS dump time.

I remember we have such API to query if flag -Xshare:dump or -Xshare:on used. Do you mean if DumpLoadedClassList flag set? This flag is the one used to log class name to list file. In GenerateLinkOptData.gmk:

$(CLASSLIST_FILE): $(INTERIM_IMAGE_DIR)/bin/java$(EXE_SUFFIX) $(CLASSLIST_JAR)
         $(call MakeDir, $(LINK_OPT_DIR))
         $(call LogInfo, Generating $(patsubst $(OUTPUTDIR)/%, %, $@))
         $(call LogInfo, Generating $(patsubst $(OUTPUTDIR)/%, %, $(JLI_TRACE_FILE)))
         $(FIXPATH) $(INTERIM_IMAGE_DIR)/bin/java -XX:DumpLoadedClassList=$@.raw \
             -Duser.language=en -Duser.country=US \
             -cp $(SUPPORT_OUTPUTDIR)/classlist.jar \
             build.tools.classlist.HelloClasslist $(LOG_DEBUG)
         $(GREP) -v HelloClasslist $@.raw > $@.interim
         $(FIXPATH) $(INTERIM_IMAGE_DIR)/bin/java -Xshare:dump \
             -XX:SharedClassListFile=$@.interim -XX:SharedArchiveFile=$@.jsa \
             -Xmx128M -Xms128M $(LOG_INFO)
         $(FIXPATH) $(INTERIM_IMAGE_DIR)/bin/java -XX:DumpLoadedClassList=$@.raw.2 \
             -XX:SharedClassListFile=$@.interim -XX:SharedArchiveFile=$@.jsa \
             -Djava.lang.invoke.MethodHandle.TRACE_RESOLVE=true \
             -Duser.language=en -Duser.country=US \
             --module-path $(SUPPORT_OUTPUTDIR)/classlist.jar \
             -cp $(SUPPORT_OUTPUTDIR)/classlist.jar \
             build.tools.classlist.HelloClasslist \
             2> $(LINK_OPT_DIR)/stderr > $(JLI_TRACE_FILE) \
             || ( \
                 exitcode=$$? ; \
                 $(ECHO) "ERROR: Failed to generate link optimization data." \
                     "This is likely a problem with the newly built JVM/JDK." ; \
                 $(CAT) $(LINK_OPT_DIR)/stderr $(JLI_TRACE_FILE) ; \
                 exit $$exitcode \
             )
         $(GREP) -v HelloClasslist $@.raw.2 > $@

The $(JLI_TRACE_FILE) is generated with both -XX:DumpLoadedClassList and -Djava.lang.invoke.MethodHandle.TRACE_RESOLVE=true, in current implementation, DumpLoadedClassList will turn on property java.lang.invoke.MethodHandle.CDS_TRACE_RESOLVE=true. So the same output sent to stdout and log file DumpLoadedClassList specified.

Now instead of this property, using a vm interface API to query if this flag is set, I think it is better choice. But here I am NOT sure I understand your suggestion, I think there are two choices:

1) Using DumpLoadedClassList to collect TRACE_RESOLVE but not via CDS_TRACE_RESOLVE, using new API to query if DumpLoadedClassList is set

2) Do not use DumpLoadedClassList, when -Xshare:dump  collecting those name, type and holder name to regenerate holder classes?

At dump time, CDS generation relies on the classlist file, it may not trigger the generation of LF classes as at pre-run.

So choice 2) may not work as expected.

>
> One possible approach:
>
> - Add the trace methods in GenerateJLIClassesHelper class to trace these resolved members:
>    traceSpeciesType(String cn)
>    tracePregeneratedLambdaForm(String name, MethodType type, Class<?> holder)
>
>    * I'm not sure if these method should also accept the case when an exception is thrown but it's okay we add it as well to match existing implementation.
>
I will keep consistent with the current logic.
> - these trace methods will either (1) send the trace output to System.out or (2) collect speciesTypes, invokerTypes, callSiteTypes, and dmhMethods in the runtime if -Xshare:dump is specified.
>
The trace method will send the log to DumpLoadedClassList file. At dump time, we reply on the trace log lines to regenerate holder classes. But if using choice 2), the trace will be store in a list and used to generate holder classes.
> - ClassSpecializer and InvokerBytecodeGenerator will call these trace methods instead.
>
> - TRACE_RESOLVE is set when the system property is set to true.   If -Xshare:dump is enabled,  the trace methods will not write to System.out.  Does CDS want a log of these entry?  I assume not?
If DumpLoadedClassList set, we wish to get output like from TRACE_RESOLVE added to the log file. At pre-run with -XX:DumpLoadedClassList=<log> we wish can collect as most loaded classes as we can for next dump run.
>
> - jlink plugin will continue to do the parsing.  It can construct a new  class that takes the
> speciesTypes, invokerTypes, callSiteTypes, and dmhMethods parameters parsed from the trace output. jlink plugin can invoke an instance method to return Map<String, byte[]>. I suggest the key for this return map is the class name rather than a jimage entry name and the caller can map it to an jimage entry name if needed.
I just move the parsing part to GenerateJLIClassesHelper, if keep the code in jlink plugin, we need a call interface from GenerateJLIClassesHelper and GenerateJLIPlugin to get the parse result, is that right? So the chain of calls will be cds(vm) -> GenerateJLIClassesHelper.generateMHHolderClasses -> JlinkPlugin parse the lines to get name, types -> GenerateJLIClassesHelper (via JLI) generate classes. My question is for a package java.lang.invoke to access jdk.internal.jlink looks not right.
>
> - CDS will invoke a different entry to return Object[] as you currently have.
>
Please let me know your comments next so I could go for a new patch.


Thanks

Yumin


> What do you think?
>
> Mandy