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

[Mandy Chung]

On 8/19/20 10:14 PM, Yumin Qi wrote:
>
> 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.
>

These entries are duplicated in two different files: one for jlink 
--generate-jli-classes plugin and another for CDS use.  CDS -Xshare:dump 
attempts to do what jlink plugin does but writes those generated classes 
in to shared archive.

Like the above make logic to build JDK image, the same entries are 
written in both default-jli-trace.txt via System.out and to classlist 
via JNI call to the VM.  I guess VM also implements the logic to do some 
kind of diffing and write to CDS archive.

> 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?
>

I misunderstood that this CDS_TRACE_RESOLVE flag is set during 
-Xshare:dump time.

Ioi has clarified to me offline that this step is actually part of 
-XX:DumpLoadedClassList and includes these TRACE_RESOLVE logs to the 
given class list file, i.e. you repurpose the class list file to include 
the log output that was initially designed for jlink plugin.

To me, I'd prefer to see this support depending on `jlink 
--generate-jli-classes` which is an existing functionality and much 
cleaner.  This way this does not require any VM change.   It will 
generate the holder classes in the custom image per the 
application-specific config file.

What it means is that: a customer would need to create a custom image 
with their application-specific config file.  It might need a new CDS  
option to specify a separate TRACE_RESOLVE file.   It would turn on this 
feature by default by defining a default path of the log file if it helps.

I understand that this is not the existing CDS work flow and CDS archive 
does not require to run on a custom image.   I see the value of this 
approach which can prepare customers to start building and using its own 
custom image.

Of course the implementation would be much simpler (adding a flag to 
write these traces to a given file path, that is).

Mandy