RFR: 8247536: Support for pre-generated java.lang.invoke classes in CDS static archive
Ioi Lam
ioi.lam at oracle.com
Mon Aug 24 23:23:10 UTC 2020
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 core-libs-dev
mailing list