RFR: 8178349: Cache builtin class loader constraints to avoid re-initializing itable/vtable for shared classes
Yumin Qi
yumin.qi at oracle.com
Wed Apr 29 23:09:59 UTC 2020
Hi, Ioi
Thanks for review, will update on your comments.
Yumin
On 4/29/20 4:04 PM, Ioi Lam wrote:
> 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