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