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:39:24 UTC 2020


Hi David.

Thanks.

-Sundar

On 18/08/20 8:04 am, David Holmes wrote:
> Hi Sundar,
>
> On 18/08/2020 12:25 pm, sundararajan.athijegannathan at oracle.com wrote:
>> 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
>
> Hotspot files do not use the Classpath exception. That is for .java 
> sources that will be"linked against" by applications.
>
> Cheers,
> David
>
>> * 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 hotspot-runtime-dev mailing list