Hi, Mandy, Ioi and Calvin
I have updated the new changed at:
http://cr.openjdk.java.net/~minqi/2020/8247536/webrev-03/
In this version:
1) Added a new API to check if flag DumpLoadedClassList set and the file is
open. If true, call into vm to print out the trace line to the log file.
Just thinking if we just call the cdsTraceResolve without checking if
the flag DumpLoadedClassList set and file open, this way, the check logic is in
the vm side like before, so save code by not adding the new API.
2) The returned holder class names now are just 'package/className',
removed head and tail.
3) Remove add_extra_classes from CollectClassesClosure since after bug
8250990: https://bugs.openjdk.java.net/browse/JDK-8250990 pushed, the
CollectClassesClosure no longer exist.
4) Still keep the parsing for TRACE_RESOLVE in
java/lang/invoke/GenerateJLIClassesHelper.java, so VM call its function to
regenerate holder classes.
Re-tested Mach5 tier1-4
Thanks
Yumin
**
On 8/20/20 8:05 PM, Yumin Qi wrote:
Hi, Mandy
On 8/20/20 5:10 PM, Mandy Chung wrote:
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.
In current implementation, vm side only records the line as from TRACE_RESOLVE
at pre-run with -XX:DumpLoadedClassList, and at dump time, call back to java
for parsing those recordings and generating the holder classes, this uses the
existing JLI code.
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.
So for now, I would implement an API to query if flag DumpLoadedClassList set
in cmd line, remove new added property of
java.lang.invoke.MethodHandle.CDS_TRACE_RESOLVE. We can address custom image
with CDS in future in a separate issue.
Thanks
Yumin
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
