RFR: 8178349: Cache builtin class loader constraints to avoid re-initializing itable/vtable for shared classes
Ioi Lam
ioi.lam at oracle.com
Fri May 1 04:13:31 UTC 2020
Hi Yumin,
Besides Calvin's suggestions, the new webrev looks good to me.
Thanks
- Ioi
On 4/30/20 2:45 PM, Yumin Qi wrote:
> Thanks, Calvin.
>
> Will update your suggestion.
> My mail client has problem, and I thought this email did not send
> out. There is another email is out, please ignore it.
>
>
> Thanks
> Yumin
>
> On 4/30/20 2:17 PM, Calvin Cheung wrote:
>> Hi Yumin,
>>
>> Looks good!
>>
>> Some minor nits on systemDictionaryShared.cpp:
>>
>> 1534 ResourceMark rm;
>>
>> You can change the above to:
>>
>> ResourceMark rm(THREAD);
>>
>> Same applies to line 1557:
>>
>> 1557 ResourceMark rm;
>>
>> No need to generate another webrev for the above change.
>>
>> One more comment below...
>>
>> On 4/30/20 1:00 PM, Yumin Qi wrote:
>>> HI, Ioi
>>>
>>> Updated webrev: http://cr.openjdk.java.net/~minqi/8178349/webrev-04/
>>>
>>> add check NULL for constraint name here like what done for
>>> verifier constraint name/from_name:
>>>
>>> 1125 if (ld._name != NULL) {
>>> 1126 ld._name->decrement_refcount();
>>> 1127 }
>>>
>>>
>>> Per Calvin's suggestion add line 116 to LoaderConstraintsApp.java:
>>>
>>> 115 MyHttpHandlerB.test(handlerC);
>>> 116 throw new
>>> RuntimeException("MyHttpHandlerB.test() did not fail as expected");
>>>
>>> Also make change to test/hotspot/jtreg/TEST.groups to include
>>> DynamicLoaderConstraintsTest.java in dynamic test group.
>>
>> The change in TEST.groups actually excludes the
>> DynamicLoaderContraintsTest.java test from the hotspot_appcds_dynamic
>> test group.
>>
>> thanks,
>>
>> Calvin
>>
>>>
>>> See embedded for answers.
>>>
>>> Retested locally for appcds.
>>>
>>> Thanks
>>> Yumin
>>>
>>> On 4/29/20 4:09 PM, Yumin Qi wrote:
>>>> 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
>>>>>
>>>
>>> Done, add ResourceMark here and other place:
>>>
>>> 1396 if (log.is_enabled()) {
>>> 1397 ResourceMark rm; 1408 if (log.is_enabled()) {
>>> 1409 ResourceMark rm; 1556 if (log.is_enabled()) {
>>> 1557 ResourceMark rm;
>>>
>>>
>>> Fixed several log indentations.
>>>>> (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) {
>>>>>
>>> added
>>>>> (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.
>>>>>
>>> fixed.
>>>>> (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)
>>>>>
>>> changed.
>>>>> 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