RFR: 8178349: Cache builtin class loader constraints to avoid re-initializing itable/vtable for shared classes
Ioi Lam
ioi.lam at oracle.com
Wed Apr 29 23:04:44 UTC 2020
HI Yumin,
Looks good!. I just have some minor nits:
systemDictionaryShared.cpp:
(1) there are several places in this file where logging requires
temporary allocation of strings (e.g., when you call
Klass::external_name). A ResourceMark is needed:
1389 if (log.is_enabled()) {
+ ResourceMark rm;
1390 // Use loader[0]/loader[1] to be consistent with the logs
in loaderConstraints.cpp
1391 log.print("[CDS record loader constraint for class: %s
constraint_name: %s "
"loader[0]: %s loader[1]: %s already added]",
1392 _klass->external_name(), name->as_C_string(),
More indentation is needed on line 1392-1394
(2) I think we should add some comments about the return value of this
function:
// returns true IFF there's no need to re-initialize the i/v-tables for
klass for
// the purpose of checking class loader constraints.
1505 bool
SystemDictionaryShared::check_linking_constraints(InstanceKlass* klass,
TRAPS) {
(3) Wording of the logs:
1523 log.print("[CDS add loader constraint for class %s symbol
%s load[0] %s loader[1] %s",
>> "CDS add loader constraint for class %s symbol %s loader[0] %s
loader[1] %s",
1544 if (log.is_enabled()) {
1545 log.print("[CDS add loader constraint for class %s is empty]",
klass->external_name());
1546 }
>> "[CDS has not recorded loader constraint for class %s]"
1547 return false; // shared class which has no loader constraints
recorded.
>> The comment is not needed as it's explained by the above log.
(3) We actually have an existing bug, where the recorded symbols are
decremented when the DumpTimeSharedClassInfo is freed (when classes are
GC'ed, mostly during dynamic dumping)
1105 if (constraint._name != NULL ) {
1106 constraint._name->decrement_refcount();
1107 }
but we forgot to increment the refcound when the symbols are recorded:
85 struct DTVerifierConstraint {
86 Symbol* _name;
87 Symbol* _from_name;
88 DTVerifierConstraint() : _name(NULL), _from_name(NULL) {}
89 DTVerifierConstraint(Symbol* n, Symbol* fn) : _name(n),
_from_name(fn) {}
So we need to add
DTVerifierConstraint(Symbol* n, Symbol* fn) : _name(n), _from_name(fn) {
_name.increment_refcount();
_from_name.increment_refcount();
}
(same for the new DTLoaderConstraint code)
Thanks
- Ioi
On 4/28/20 9:38 PM, Yumin Qi wrote:
> HI, Ioi
>
> Thanks for the review, update webrev at:
> http://cr.openjdk.java.net/~minqi/8178349/webrev-03/
> <http://cr.openjdk.java.net/~minqi/8178349/webrev-02/>
> Thanks for working with the test case.
>
> New test cases LoaderConstraintsTest.java and
> DynamicLoaderConstraintsTest.java added.
> There is a bug in the code which has been identified by the newly
> added DynamicLoaderConstraintsTest.java. For the shared class, if
> archived loader constraints exist and add_loader_constraints succeed,
> means no need to relay out the i/vtable calls, return "true" but in
> the code, I wrongly placed the "return true" outside if
> (info->_num_loader_constraints) { ... }
>
> 1536 bool
> SystemDictionaryShared::check_linking_constraints(InstanceKlass*
> klass, TRAPS) {
> 1537 assert(!DumpSharedSpaces && UseSharedSpaces, "called at run time
> with CDS enabled only");
> 1538 if (klass->is_shared_boot_class()) {
> 1539 // No class loader constraint check performed for boot classes.
> 1540 return true;
> 1541 }
> 1542 if (klass->is_shared_platform_class() ||
> klass->is_shared_app_class()) {
> 1543 RunTimeSharedClassInfo* info =
> RunTimeSharedClassInfo::get_for(klass);
> 1544 assert(info != NULL, "Sanity");
> 1545 if (info->_num_loader_constraints > 0) {
> 1546 HandleMark hm;
> 1547 for (int i = 0; i < info->_num_loader_constraints; i++) {
> 1548 RunTimeSharedClassInfo::RTLoaderConstraint* lc =
> info->loader_constraint_at(i);
> 1549 Symbol* name = lc->constraint_name();
> 1550 Handle loader1(THREAD, get_class_loader_by(lc->_loader_type1));
> 1551 Handle loader2(THREAD, get_class_loader_by(lc->_loader_type2));
> 1552 if (!SystemDictionary::add_loader_constraint(name, klass,
> loader1, loader2, THREAD)) {
> 1553 // Loader constraint violation has been found. The caller
> 1554 // will re-layout the vtable/itables to produce the correct
> 1555 // exception.
> 1556 return false;
> 1557 }
> 1558 }
> 1559 }
> 1560 return true; // for all recorded constraints added or no
> constraints recorded at all <----- should move one line up.
> 1561 }
> 1562 return false; // shared custom class which has no constraints
> recorded.
> 1563 }
>
>
> One minor change for cleaning existing code is that removed
> get_constraint_name(int i) and get_constraint_from_name(int i) from
> RTVerfierConstraint. We already have member functions of
> RTVerifierConstraint* verifier_constraints() and
> RTVerfierConstraints* verifier_constraint_at(int i)
> So add member functions to RTVerifierConstraint to get name and
> from_name make much sense:
>
> 240 Symbol* name() { return (Symbol*)(SharedBaseAddress + _name);}
> 241 Symbol* from_name() { return (Symbol*)(SharedBaseAddress +
> _from_name); }
> Re-tested hs-tier1-4. (there are two failures, but they are not
> related to the change). local test for jtreg on runtime. Please check
> embedded for Answers to your questions.
>
> Thanks
> Yumin
>
>
> On 4/9/20 4:29 PM, Ioi Lam wrote:
>> Hi Yumin,
>>
>> This looks good!
>>
>> I have some suggestions so we can describe the design inside
>> the code and make the optimization easier to understand.
>>
>>
>> (Also, I think we need more test cases. I'll work with you separately
>> on that)
>>
>>
>> (1) Before your change, the JVM checks loader constraints in two places:
>> (a) during linking of a class
>> (b) when resolving constant pool entries.
>>
>> Since your change involves only (a), I think it's better to rename
>> the function SystemDictionaryShared::record_loader_constraint ->
>> record_linking_constraint, and add these comments:
>>
>> // Record class loader constraints that are checked inside
>> // InstanceKlass::link_class(), so that these can be checked quickly
>> // at runtime without laying out the vtable/itables.
>> void SystemDictionaryShared::record_linking_constraint(...)
>>
> Done
>> (2) SystemDictionary::check_signature_loaders: parameter "this_klass"
>> can be
>> changed to "class_being_linked". When calling this function from
>> linkResolver.cpp, we can say
>>
>> /*class_being_linked=*/ NULL, // we are not linking a class
>>
> Done
>> (3) We can restructure the checks in
>> SystemDictionary::add_loader_constraint
>> to make them easier to understand:
>>
>> FROM:
>>
>> 2289 // record constraint for app/platform-loader loaded class only.
>> 2290 // dynamic dumping will re-layout of the vtable of the
>> *copy* of a class
>> 2291 // in a vm_operation, the loader constraints alread recorded.
>> 2292 if (Arguments::is_dumping_archive() &&
>> !THREAD->is_VM_thread()) {
>> 2293 if (this_klass != NULL && !this_klass->is_shared() &&
>> 2294 (is_system_class_loader(this_klass->class_loader()) ||
>> 2295 is_platform_class_loader(this_klass->class_loader()))) {
>> 2296 SystemDictionaryShared::record_loader_constraint(constraint_name,
>> 2297 InstanceKlass::cast(this_klass),
>> 2298 class_loader1, class_loader2);
>> 2299 }
>> 2300 }
>>
>> TO:
>>
>> if (Arguments::is_dumping_archive() && class_being_linked != NULL &&
>> !this_klass->is_shared()) {
>> SystemDictionaryShared::record_linking_constraint(constraint_name, ...);
>> }
>>
>> All the conditions that are specific to the linking constraints
>> optimization should be moved into the CDS code.
>>
>> In the beginning of record_linking_constraint():
>>
>> void SystemDictionaryShared::record_linking_constraint(
>> // A linking constraint check is executed when:
>> // - klass extends or implements type S
>> // - klass overrides method S.M(...)
>> // - loader1 = klass->class_loader()
>> // - loader2 = S->class_loader()
>> // - loader1 != loader2
>> // - M's paramater(s) include an object type T
>> // We require that
>> // - whenever loader1 and loader2 try to
>> // resolve the type T, they must always resolve to
>> // the same InstanceKlass.
>> // NOTE: type T may or may not be currently resolved in
>> // either of these two loaders. The check itself does not
>> // try to resolve T.
>>
>> assert(klass->class_loader() != NULL,
>> "should not be called for boot loader");
> Done
>> assert(klass->class_loader() == loader1(), "must be");
> This one not always stands, like we discussed in separate thread. The
> interface of the class may loaded by other loader than the class
> loader. Or the class super loader may differ from the class loader. It
> failed LotsOfClasses and dynamicArchive/DynamicLotsOfClasses.java.
>>
>> assert(loader1() != loader2(), "must be");
>>
>> if (!is_system_class_loader(klass->class_loader() &&
>> !is_platform_class_loader(klass->class_loader()) {
>> // If klass is loaded by system/platform loaders, we can
>> // guarantee that klass and S must be loaded by the same
>> // respective loader between dump time and run time, and
>> // the exact same check on (name, loader0, loader1) will
>> // be executed. Hence, we can cache this check and execute
>> // it at runtime without walking the vtable/itables.
>> //
>> // This cannot be guaranteed for classes loaded by other
>> // loaders, so we bail.
>> return;
>> }
>>
>> if (THREAD->is_VM_thread()) {
>> assert(DynamicDumpSharedSpaces, "must be");
>> // We are re-laying out the vtable/itables of the *copy* of
>> // a class during the final stage of dynamic dumping. The
>> // linking constraints for this class has already been recorded.
>> return;
>> }
>>
> done
>> (4) Similarly, for the runtime check, we can move the loader type
>> check into
>> the CDS code:
>>
>> InstanceKlass::link_class_impl() {
>> ...
>> bool need_init_table = true;
>> if (is_shared() &&
>> SystemDictionaryShared::check_linking_constraints(this, THREAD)) {
>> need_init_table = false;
>> }
>> if (need_init_table) {
>> vtable().initialize_vtable(true, CHECK_false);
>> itable().initialize_itable(true, CHECK_false);
>> }
>> ...
>> }
>>
>>
>> // Returns true IFF the linking constraints have been checked and no
>> // violations have been found.
>> bool
>> SystemDictionaryShared::check_linking_constraints(InstanceKlass*
>> klass, TRAPS) {
>> assert(klass->is_shared(), "must be");
>> if (... is boot class ..) {
>> // No class loader constraints checks are performed for boot
>> classes.
>> return true;
>> }
>> if (not (... is platform or system ...)) {
>> // linking constraints are not recorded for these classes.
>> return false;
>> }
>>
>> ....
>> if (!SystemDictionary::add_loader_constraint(....)) {
>> // Loader constraint violation has been found. The caller
>> // will re-layout the vtable/itables to produce the correct
>> // exception.
>> return false;
>> }
>> ....
>> }
>>
>> I changed the meaning of check_linking_constraints() so it's
>> more natural:
>>
>> true means "good - no need to check"
>> false means "bad - need to check more")
>>
> Done.
>> (5) Also, for DTLoaderConstraint::operator==, we usually avoid operator
>> overloading in the HotSpot code, so I think it's better to rename
>> it to
>> DTLoaderConstraint::equals().
>>
> Done.
>> Thanks
>> - Ioi
>>
>>
>>
>>
>> On 4/9/20 10:06 AM, Yumin Qi wrote:
>>> Hi, please review (please ignore my last email, the Subject is not
>>> correctly titled).
>>>
>>> bug: https://bugs.openjdk.java.net/browse/JDK-8178349
>>> webrev: http://cr.openjdk.java.net/~minqi/8178349/webrev-01/
>>>
>>> Summary: When CDS link class, need re- intialize_vtable and
>>> intialize_itable. For a shared class the run for initialize table(s)
>>> in fact just check signature loaders of virtual or interface
>>> functions but we need go through many steps to reach the functions,
>>> iterate over the functions for signatures, resolve the symbol in
>>> loaders. Those steps also involved lock for SystemDictionary many
>>> times for resolving
>>> classes. (note for boot loaded shared classed no need to rerun).
>>> This patch record the loader constraints during dump time and in
>>> runtime, directly check loader constraints for the shared class
>>> (loaded by app or platform loader). The performance data showed:
>>>
>>> Results of " perf stat -r 40 bin/javac -J-Xshare:on
>>> -J-XX:SharedArchiveFile=javac2.jsa Bench_HelloWorld.java "
>>> 1: 2800027008 2793677554 ( -6349454) -- 390.665
>>> 390.210 ( -0.455)
>>> 2: 2799894892 2801783826 ( 1888934) 392.640
>>> 389.890 ( -2.750) ---
>>> 3: 2808688341 2792077758 (-16610583) ---- 392.430
>>> 390.210 ( -2.220) --
>>> 4: 2806708208 2790107502 (-16600706) ---- 395.210
>>> 390.060 ( -5.150) -----
>>> 5: 2807039108 2791228262 (-15810846) ---- 392.470
>>> 388.870 ( -3.600) ---
>>> 6: 2803446643 2783022861 (-20423782) ----- 392.550
>>> 388.070 ( -4.480) ----
>>> 7: 2809043394 2796696131 (-12347263) --- 394.810
>>> 389.960 ( -4.850) -----
>>> 8: 2798698381 2788160443 (-10537938) --- 393.430
>>> 390.130 ( -3.300) ---
>>> 9: 2797579793 2789786208 ( -7793585) -- 392.330
>>> 390.204 ( -2.126) --
>>> 10: 2808294825 2800296275 ( -7998550) -- 394.180
>>> 390.230 ( -3.950) ----
>>> ============================================================
>>> 2803938765 2792678505 (-11260259) --- 393.069
>>> 389.783 ( -3.287) ---
>>> instr delta = -11260259 -0.4016%
>>> time delta = -3.287 ms -0.8361%
>>>
>>> Results of " perf stat -r 40 bin/java -Xshare:on
>>> -XX:SharedArchiveFile=zprint2.jsa -cp ./zprint-filter-fixed.jar
>>> ZPrintBench "
>>> 1: 6548556888 6376039292 (-172517596) ---- 731.250
>>> 708.030 (-23.220) -----
>>> 2: 6520993435 6391732011 (-129261424) --- 729.150 711.350
>>> (-17.800) ----
>>> 3: 6505744758 6355633193 (-150111565) ---- 729.440
>>> 707.090 (-22.350) -----
>>> 4: 6545887467 6362941735 (-182945732) ----- 731.250
>>> 706.680 (-24.570) -----
>>> 5: 6497462252 6363834813 (-133627439) --- 729.940 709.540
>>> (-20.400) ----
>>> 6: 6567639848 6369214232 (-198425616) ----- 733.770
>>> 709.550 (-24.220) -----
>>> 7: 6511495888 6369700928 (-141794960) ---- 729.380
>>> 710.430 (-18.950) ----
>>> 8: 6531525105 6342720734 (-188804371) ----- 730.070
>>> 706.090 (-23.980) -----
>>> 9: 6508499676 6322714877 (-185784799) ----- 728.700
>>> 705.560 (-23.140) -----
>>> 10: 6532457875 6430536065 (-101921810) --- 731.590 718.240
>>> (-13.350) ---
>>> ============================================================
>>> 6526992135 6368448595 (-158543539) ---- 730.453
>>> 709.247 (-21.205) ----
>>> instr delta = -158543539 -2.4290%
>>> time delta = -21.205 ms -2.9030%
>>>
>>> Tests: hs-tier1,2,3,4
>>>
>>> Thanks
>>> Yumin
>>
>
More information about the hotspot-runtime-dev
mailing list