RFR: 8247536: Support for pre-generated java.lang.invoke classes in CDS static archive
Coleen Phillimore
coleen.phillimore at oracle.com
Tue Aug 25 19:41:14 UTC 2020
Hi, I'm sorry I haven't been watching this change.
http://cr.openjdk.java.net/~minqi/2020/8247536/webrev-04/src/hotspot/share/classfile/classLoaderData.cpp.udiff.html
Can you move find_class() into systemDictionary.hpp/cpp with the rest of
the find_class(). This is strange here. You don't want to expose that
the dictionary is here to the rest of the code. The caller should call
one of the SystemDictionary versions, or add this to SystemDictionary
with the other functions with the same name.
http://cr.openjdk.java.net/~minqi/2020/8247536/webrev-04/src/hotspot/share/memory/lambdaFormInvokers.cpp.html
These new files belongs in classfile. It's closer to
systemDictionaryShared code than memory code.
2 * Copyright (c) 2012, 2020, Oracle and/or its affiliates. All rights reserved.
Also copyright shouldn't start with 2012.
http://cr.openjdk.java.net/~minqi/2020/8247536/webrev-04/src/hotspot/share/classfile/systemDictionaryShared.cpp.udiff.html
This add_to_hierarchy is a strange delegation and a surprise that it's
also defined here. Please call this inline from lambdaFormInvokers, ie
take the Compile_lock and call SystemDictionary::add_to_hierarchy().
I don't have any deeper comments.
Thanks,
Coleen
On 8/25/20 2:59 PM, Yumin Qi wrote:
> HI, Ioi
>
> Thanks for the re-review, updated webrev:
>
> http://cr.openjdk.java.net/~minqi/2020/8247536/webrev-04/
>
> Changed according to your suggestion, the only a little difference
> is move traceResolve(String line) to InvokerBytecodeGenerator as a
> static package public function so other package classes can call it.
>
> re-tested local on runtime/cds
>
>
> Thanks
>
> Yumin
>
>
> On 8/24/20 4:23 PM, Ioi Lam wrote:
>> Hi Yumin,
>>
>> This looks good overall. Here are my comments:
>>
>> =====================
>> 6065 size_t new_id = Atomic::add(&counter, (size_t)1);
>> 6066 jio_snprintf(addr_buf, 20, INTPTR_FORMAT, new_id);
>>
>> I think this should be SIZE_FORMAT
>>
>> =====================
>> 65 class KlassFactory : AllStatic {
>> 66
>> 67 // approved clients
>> 68 friend class ClassLoader;
>> 69 friend class ClassLoaderExt;
>> 70 friend class SystemDictionary;
>> 71 friend class LambdaFormInvokers;
>> 72
>> 73 private:
>> 74 static InstanceKlass* create_from_stream(ClassFileStream* stream,
>>
>> I think instead of adding everyone who uses create_from_stream as a
>> friend class, we should just change create_from_stream into a public
>> function and remove the friend declarations.
>>
>> =====================
>> 146 // add to hierarchy and set state to loaded.
>> 147 {
>> 148 MutexLocker mu_r(THREAD, Compile_lock); //
>> add_shared_to_hierarchy asserts this.
>> 149 SystemDictionaryShared::add_shared_to_hierarchy(result,
>> THREAD);
>> 150 }
>>
>> I think the function name can be changed to
>> SystemDictionaryShared::add_to_hierarchy as the "_shared" seems
>> redundant. The "set state to loaded" comment seems wrong, as we have
>> the assert on line 1155. I think the comment can be removed.
>>
>> 1153 void
>> SystemDictionaryShared::add_shared_to_hierarchy(InstanceKlass* k,
>> TRAPS) {
>> 1154 Arguments::assert_is_dumping_archive();
>> 1155 assert(!k->is_loaded(), "Invariant");
>> 1156 assert_locked_or_safepoint(Compile_lock); // add_to_hierarchy
>> assert on it.
>> 1157 SystemDictionary::add_to_hierarchy(k, CHECK);
>> 1158 }
>>
>> Also, I think it's better to move the MutexLocker call into
>> SystemDictionaryShared::add_shared_to_hierarchy.
>>
>> ========================
>>
>> before:
>>
>> 478 if (TRACE_RESOLVE && salvage != null) {
>> 479 // Used by jlink species pregeneration
>> plugin, see
>> 480 //
>> jdk.tools.jlink.internal.plugins.GenerateJLIClassesPlugin
>> 481 System.out.println("[SPECIES_RESOLVE] " +
>> className + " (salvaged)");
>> 482 }
>>
>> after:
>>
>> 488 // Used by jlink species pregeneration plugin, see
>> 489 //
>> jdk.tools.jlink.internal.plugins.GenerateJLIClassesPlugin
>> 490 traceResolve("[SPECIES_RESOLVE] " + className +
>> " (salvaged)");
>>
>>
>> When tracing is disabled, this will make extra allocations and cause
>> a small slowdown. I think it's better to
>>
>> if ((TRACE_RESOLVE|TRACE_RESOLVE_CDS) && salvage != null) {
>> traceResolve("[SPECIES_RESOLVE] " + className + " (salvaged)");
>> }
>>
>> Because TRACE_RESOLVE is a static final boolean, the JIT compiler
>> will completely optimize this block out.
>>
>> For the same reason, instead of calling
>> VM.isDumpLoadedClassListSetAndOpen() every time, it's better to use a
>> static final variable.
>>
>>
>> =======================
>>
>> 698 if (TRACE_RESOLVE) {
>> 699 System.out.println("[LF_RESOLVE] " +
>> holder.getName() + " " + name + " " +
>> 700 shortenSignature(basicTypeSignature(type)) + (resolvedMember !=
>> null ? " (success)" : " (fail)") );
>> 701 }
>> 702 if (VM.isDumpLoadedClassListSetAndOpen()) {
>> 703 GenerateJLIClassesHelper.cdsTraceResolve("[LF_RESOLVE] " +
>> holder.getName() + " " + name + " " +
>> 704 shortenSignature(basicTypeSignature(type)) + (resolvedMember !=
>> null ? " (success)" : " (fail)") );
>> 705 }
>> 706 return resolvedMember;
>>
>>
>> I think the two "if" blocks should be combined similarly to
>> ClassSpecializer::traceResolve().
>>
>> =========================
>> 34
>> Java_java_lang_invoke_GenerateJLIClassesHelper_cdsTraceResolve(JNIEnv
>> *env, jclass ignore, jstring line) {
>>
>> Maybe this should be moved to the "VM" class as well?
>>
>> =========================
>> lambdaFormInvokers.hpp:
>>
>> Need these declarations:
>>
>> #include "memory/allocation.hpp" <-- for AllStatic
>> #include "runtime/handles.hpp" <-- for typeArrayHandle and
>> Handle
>> #include "utilities/exceptions.hpp" <-- for TRAPS
>>
>> template <class T> class GrowableArray;
>> =========================
>>
>>
>> Thanks
>> - Ioi
>>
>>
>>
>> On 8/23/20 3:56 PM, Yumin Qi wrote:
>>>
>>> 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
>>
More information about the hotspot-runtime-dev
mailing list