RFR: 8247536: Support for pre-generated java.lang.invoke classes in CDS static archive

Sun Aug 16 01:27:38 UTC 2020

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
>>>
>>>   `
>