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...@oracle.com>
To: hotspot-runtime-...@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
`
