RFR: 8247536: Support for pre-generated java.lang.invoke classes in CDS static archive
sundararajan.athijegannathan at oracle.com
sundararajan.athijegannathan at oracle.com
Tue Aug 18 02:25:21 UTC 2020
Not a full review of fresh changes. But couple of comments:
* src/hotspot/share/memory/lambdaFormInvokers.cpp and
src/hotspot/share/memory/lambdaFormInvokers.hpp miss "Classpath
exception" clause in the copyright header
* I had suggested a change GenerateJLIClassesPlugin.java in last round
of review. That's still applicable.
https://mail.openjdk.java.net/pipermail/core-libs-dev/2020-August/068160.html
-Sundar
On 18/08/20 1:07 am, 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/
>
> Following changes done in this patch:
>
> 1) move regenerating holder classes into new added file:
> lambdaFormInvokers.[ch]pp
>
> "@lambda-form-invoker" tag only observed at jvm side.
>
> 2) remove InvokerBytecodeGeneratorHelper.java, move related
> functions to existing class, GenerateJLIClassesHelper.java
>
> 3) add tracing for SPECIES_RESOLVE, so archive more classes.
>
> 4) remove InvokerGenerateBytesException.java, using RuntimeException
> instead.
>
> 5) Changed make file to exclude the new added lamdaFormInvokers.cpp
> from the non-cds building list.
>
> 6) There is a bug in previous patch, jobject (typeArrayOop) should
> not be used directly for loading class since GC will move the java
> object. Fixed by copying the bytes from handle to C heap.
>
> 7) Tested with tier1,2
>
>
> Thanks
>
> Yumin
>
>
>
> On 8/15/20 6:27 PM, Ioi Lam wrote:
>> On 8/15/20 6:19 PM, Ioi Lam wrote:
>>> To better capture what we're trying to do in this RFE, I've changed
>>> the RFE title (and the subject of this email thread) to
>>>
>>> https://bugs.openjdk.java.net/browse/JDK-8247536
>>> Support for pre-generated java.lang.invoke classes in CDS static
>>> archive
>>>
>>> I also update the RFE Description to give an overview of the problem
>>> and the solution.
>>>
>>> The design document is also updated to reflect Yumin's latest
>>> implementation
>>>
>>
>> Oops, sent too quickly. The design doc's title is also updated:
>>
>> https://wiki.openjdk.java.net/display/HotSpot/Support+for+pre-generated+java.lang.invoke+classes+in+CDS+archive
>>
>>
>>> Thanks
>>> - Ioi
>>>
>>> On 8/12/20 11:54 AM, calvin.cheung at oracle.com wrote:
>>>> Hi Yumin,
>>>>
>>>> I reviewed mostly the native code. Below are my comments:
>>>>
>>>> 1) classListParser.hpp
>>>>
>>>> 71 bool _lambda_format;
>>>>
>>>> The above name is too generic. How about _lambda_form or
>>>> _is_lambda_form?
>>>> If you rename the above, please also rename the function which
>>>> returns the above bool.
>>>>
>>>> 2) jvm.cpp
>>>>
>>>> 3850 JVM_ENTRY(void, JVM_CDSTraceResolve(JNIEnv* env, jclass
>>>> ignore, jstring line))
>>>>
>>>> ignore -> ignored
>>>>
>>>> 3) jvm.hpp
>>>>
>>>> 210 JVM_CDSTraceResolve(JNIEnv* env, jclass ignore, jstring line);
>>>>
>>>> Same comment as for jvm.cpp
>>>>
>>>> 4) metaspaceShared.cpp
>>>>
>>>> 2017 size_t i = 0;
>>>> 2018 while (i < size) {
>>>> 2019 full_name[i++] = *start++;
>>>> 2020 }
>>>>
>>>> Could the above be simplified to the following?
>>>>
>>>> strncpy(full_name, start, size - 1);
>>>>
>>>> 2029 char* class_name = get_full_class_name(path_name);
>>>>
>>>> Should os::free(class_name) be called before the function returns?
>>>>
>>>> 1870 static GrowableArray<char *>* lambda_list = NULL;
>>>>
>>>> The name lambda_list is a bit generic, how about lambda_form_list?
>>>>
>>>> 2112 lambda_list->append(parser.current_line());
>>>>
>>>> Since parser.current_line() does a strdup, do those buffer need to
>>>> be freed after its use? (i.e. after the call to
>>>> regenerate_holder_classes()?)
>>>>
>>>> In MetaspaceShared::regenerate_holder_classes, before calling up to
>>>> java, I think it's better to strip the prefix
>>>> "@lambda-form-invoker" from the strings. So that the java
>>>> InvokerBytecodeGeneratorHelper.readTraceConfig method doesn't need
>>>> to handle it:
>>>>
>>>> 143 case "[LF_RESOLVE]":
>>>> 144 case InvokerBytecodeGenerator.CDS_LOG_PREFIX:
>>>>
>>>> 5) DumpSymbolAndStringTable.java
>>>>
>>>> 37 private static final String my_string =
>>>> "DumpSymbolAndStringTable";
>>>>
>>>> Unused variable?
>>>>
>>>> thanks,
>>>>
>>>> Calvin
>>>>
>>>> On 8/11/20 10:36 AM, Yumin Qi wrote:
>>>>> Forget to send to @core-lib-dev, the patch changed jdk code.
>>>>>
>>>>>
>>>>> Thanks
>>>>>
>>>>> Yumin
>>>>>
>>>>>
>>>>>
>>>>> -------- Forwarded Message --------
>>>>> Subject: RFR: 8247536: Support pre-generated MethodHandle
>>>>> lambda forms in CDS
>>>>> Date: Tue, 11 Aug 2020 07:44:34 -0700
>>>>> From: Yumin Qi <yumin.qi at oracle.com>
>>>>> To: hotspot-runtime-dev at openjdk.java.net
>>>>>
>>>>>
>>>>>
>>>>> Hi, Please reivew
>>>>>
>>>>> bug: https://bugs.openjdk.java.net/browse/JDK-8247536
>>>>> Webrev: http://cr.openjdk.java.net/~minqi/2020/8247536/webrev-01/
>>>>>
>>>>> Summary: CDS does not archive pre-generated lambda form classes
>>>>> for method handles:
>>>>>
>>>>> [0.142s][info][class,load]
>>>>> java.lang.invoke.LambdaForm$MH/0x0000000800066c40 source:
>>>>> __JVM_LookupDefineClass__
>>>>>
>>>>> The method handle resolution if not found in the holder class, a
>>>>> class with the method will be generated and loaded as vm internal
>>>>> created class and not archived like above. Those class names are
>>>>> mangled as combination of the class name and the class instance
>>>>> address.
>>>>>
>>>>> In this patch, collect those method holder class names and their
>>>>> associated methods' signatures when user specify
>>>>> DumpLoadedClassList, and record them in the log file in a format
>>>>> similar to the output format from traced by
>>>>> -Djava.lang.invoke.MethodHandle.TRACE_RESOLVE=true, but here use
>>>>> another name for CDS:
>>>>> -Djava.lang.invoke.MethodHandle.CDS_TRACE_RESOLVE=true instead.
>>>>> The line prefix also changed from "[LF_RESOLVE]" to
>>>>> "@lambda-invoke-handle". At dump time, regenerate the holder
>>>>> class with those methods and replace the existing holder class and
>>>>> archived it. At runtime, the resolution of the holder class which
>>>>> contains those methods are loaded from CDS archive so avoid
>>>>> regenerating them again. The patch reorganized the code for Jlink
>>>>> plugin, so the new added InvokerBytecodeGeneratorHelper can be
>>>>> shared both by JLink plugin and CDS code. Also change made to the
>>>>> mangled generated class name at static dump, the class name
>>>>> combining the class name and a sequentially ordered number instead
>>>>> of the class instance address, which looks like a random.
>>>>>
>>>>>
>>>>> To use this feature, one can do the dump/run just like done for a
>>>>> static dump/run, so no extra steps required.
>>>>>
>>>>> 1) java -XX:DumpLoadedClassList=myclasslist JavaApp
>>>>>
>>>>> DumpLoadedClassList will turn on
>>>>> -Djava.lang.invoke.MethodHandle.CDS_TRACE_RESOLVE=true
>>>>>
>>>>> 2) java -Xshare:dump -XX:SharedClassListFile=myclasslist
>>>>> -XX:SharedArchiveFile=my.jsa JavaApp
>>>>>
>>>>> 3) java -Xshare:on -XX:SharedArchiveFile=my.jsa JavaApp
>>>>>
>>>>> The performance on javac HelloWorld.java (javac-benchmark), no
>>>>> patch vs patch:
>>>>>
>>>>> 1: 2689285002 2641821474 (-47463528) ---- 391.720
>>>>> 382.990 ( -8.730) ----
>>>>> 2: 2687495085 2632969688 (-54525397) ---- 391.030
>>>>> 381.480 ( -9.550) -----
>>>>> 3: 2694664066 2636523114 (-58140952) ----- 390.610
>>>>> 382.550 ( -8.060) ----
>>>>> 4: 2686554164 2639355233 (-47198931) ---- 390.700
>>>>> 383.390 ( -7.310) ---
>>>>> 5: 2691072338 2633016687 (-58055651) ----- 388.990
>>>>> 382.360 ( -6.630) ---
>>>>> 6: 2684448174 2644191854 (-40256320) --- 389.450
>>>>> 382.990 ( -6.460) ---
>>>>> 7: 2694921227 2630505090 (-64416137) ----- 389.300
>>>>> 383.160 ( -6.140) ---
>>>>> 8: 2685209712 2639334320 (-45875392) ---- 388.370
>>>>> 381.060 ( -7.310) ---
>>>>> 9: 2695885942 2640618655 (-55267287) ---- 389.560
>>>>> 381.100 ( -8.460) ----
>>>>> 10: 2689162942 2635658943 (-53503999) ---- 389.690
>>>>> 379.110 (-10.580) -----
>>>>> ============================================================
>>>>> 2689866989 2637396210 (-52470778) ---- 389.941
>>>>> 382.017 ( -7.924) ----
>>>>>
>>>>> instr delta = -52470778 -1.9507%
>>>>>
>>>>> time delta = -7.924 ms -2.0321%
>>>>>
>>>>> It will show much more improvement if compare with the default
>>>>> archive:
>>>>>
>>>>> no patch patch
>>>>>
>>>>> instr 3,996,847,605 3,320,928,995
>>>>>
>>>>> time(s). 0.686835162 0.474933546
>>>>>
>>>>>
>>>>> Tests:
>>>>>
>>>>> 1)jtreg on jdk/tools and hotspot/runtime local.
>>>>>
>>>>> 2) mach5 ter1,2
>>>>>
>>>>>
>>>>> Thanks
>>>>>
>>>>> Yumin
>>>>>
>>>>> `
>>>
>>
More information about the core-libs-dev
mailing list