Hi Yumin,
This looks good!
Here are my comments:
[1] classListParser.cpp:
#define LAMBDA_FOMRM_INVOKER "@lambda-form-invoker"
I think this should be moved to classListParser.hpp:
const char* lambda_form_invoker_tag() {
return "@lambda-form-invoker";
}
Also, instead of also duplicating this string in
InvokerBytecodeGenerator.java, the Java side of the code can be changed to
String outLine = holder.getName() + " " + name + " " + ....
and JVM_CDSTraceResolve() can be changed to
classlist_file->print_cr("%s %s",
ClassListParser::lambda_form_invoker_tag(), c_line);
[2] metaspaceShared.cpp:
This file is getting too big. I think we should move the new
functionality to a new file
share/memory/lambdaFormInvokers.cpp
[3] MetaspaceShared::preload_classes()
2080 if (parser.is_lambda_format())
This name is too generic. How about "parser.is_lambda_form_invoker()"?
2109 if (lambda_list == NULL) {
2110 lambda_list = new GrowableArray<char*>(8);
2111 }
2112 lambda_list->append(parser.current_line());
These can be changed to
LambdaFormInvokers::append(parser.current_line());
[4] I think the follow code that changes the system dictionary is
kind of scary. Maybe we can avoid it.
2060 // replace with the new created klass.
2061 {
2062 MutexLocker lock(THREAD, SystemDictionary_lock);
2063 InstanceKlass* old = cld->replace_class(sym, result);
2064 SystemDictionaryShared::set_excluded(old);
2065 log_info(cds)("Replace class %s, old: %p new: %p", class_name,
old, result);
2066 }
How about just do this:
InstanceKlass* old = cld->find_class(sym);
SystemDictionaryShared::set_excluded(old);
This will prevent the old class from being added to CollectClassesClosure.
We actually don't need to add the new class into cld->dictionary() -- we
don't
want to "use" the new class in any way other than writing it into the
archive.
We can do this to add the new class to the CollectClassesClosure:
+ GrowableArray* LambdaFormInvokers::replacement_holder_classes();
...
_global_klass_objects = new GrowableArray<Klass*>(1000);
CollectClassesClosure collect_classes;
ClassLoaderDataGraph::loaded_classes_do(&collect_classes);
+
collect_classes.add_extra_classes(LambdaFormInvokers::replacement_holder_classes());
_global_klass_objects->sort(global_klass_compare);
[5] InvokerGenerateBytesException.java
This class cannot be public, as it's not part of the java.lang.invoke
package specification.
Anyway, we don't seem to actually need this type, as it's not used
outside of
InvokerBytecodeGeneratorHelper. How about replacing all use of
throw new InvokerGenerateBytesException(...);
to
throw new RuntimeException(...);
[6] This adds duplicated code for processing the option:
2846 // -XX:DumpLoadedClassList
2847 } else if (match_option(option, "-XX:DumpLoadedClassList=",
&tail)) {
2848 if (tail != NULL) {
2849 add_property("java.lang.invoke.MethodHandle.CDS_TRACE_RESOLVE=true");
2850 DumpLoadedClassList = os::strdup_check_oom(tail);
2851 } else {
2852 warning("Bad option for -XX:DumpLoadedClassList=<file>");
2853 return JNI_EINVAL;
2854 }
I think it's better to move the code to here:
jint Arguments::parse(const JavaVMInitArgs* initial_cmd_args) {
....
- #if !INCLUDE_CDS
+ #if INCLUDE_CDS
+ if (DumpLoadedClassList != NULL) {
+ if
(!add_property("java.lang.invoke.MethodHandle.CDS_TRACE_RESOLVE=true")) {
+ return JNI_ENOMEM;
+ }
+ }
+ #else
[7] For testing, we should have different kinds of invalid input for the
@lambda-form-invoker to make sure that the errors are handled properly.
[8] Some variable names should be changed to be more descriptive. Here is
an example:
// k - the class full name
// v - the class bytes
void MetaspaceShared::reload_class(Handle k, Handle v, TRAPS) {
should be changed class_name, class_bytes.
Thanks
- Ioi
On 8/11/20 10:36 AM, Yumin Qi wrote:
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
`
