RFR: 8178349: Cache builtin class loader constraints to avoid re-initializing itable/vtable for shared classes
Yumin Qi
yumin.qi at oracle.com
Thu Apr 30 21:45:10 UTC 2020
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