RFR: 8178349: Cache builtin class loader constraints to avoid re-initializing itable/vtable for shared classes
Calvin Cheung
calvin.cheung at oracle.com
Thu Apr 30 21:17:12 UTC 2020
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