RFR: 8247536: Support for pre-generated java.lang.invoke classes in CDS static archive
Coleen Phillimore
coleen.phillimore at oracle.com
Tue Aug 25 21:15:26 UTC 2020
On 8/25/20 5:09 PM, Yumin Qi wrote:
> HI, Coleen
>
> Thanks for review.
>
> On 8/25/20 12:41 PM, Coleen Phillimore wrote:
>>
>> 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.
>>
> There exist two find_class functions:
>
> // Basic find on loaded classes
> static InstanceKlass* find_class(unsigned int hash,
> Symbol* name, Dictionary* dictionary);
> static InstanceKlass* find_class(Symbol* class_name,
> ClassLoaderData* loader_data);
>
> They are all protected. I think I can change the second version to
> public access? Then using it should be more convenient. I added
> find_class is because those functions are all protected in
> SystemDictionary, they are not available to public.
Yes, it would be better to make the second one available to your caller.
>
>> 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.
>>
> Oops, copy error.
>>
>> 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().
>>
> Yes, this extra layout seems not necessary --- it just indicates that
> this is from shared code. I will use
> SystemDictionary::add_to_hierarchy directly.
>
Thanks!
Coleen
>
> Thanks
>
> Yumin
>
>> 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