Fwd: RFR: 8247536: Support pre-generated MethodHandle lambda forms in CDS

Wed Aug 12 18:54:27 UTC 2020

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