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:41:41 UTC 2020
HI, Ioi
Thanks. I have updated the webrev:
http://cr.openjdk.java.net/~minqi/8178349/webrev-04/
Per Calvin's (Thanks Calvin) suggestion, have made changes:
LoaderConstraintsApp.java:
115 MyHttpHandlerB.test(handlerC);
116 throw new RuntimeException("MyHttpHandlerB.test() did not fail as expected"); // << --- new added
Add DynamicLoaderConstraintsTest.java to hotspot_appcds_dynamic test group.
See embedded answers.
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, also more places for ResourceMark needed:
1408 if (log.is_enabled()) {
1409 ResourceMark rm; 1533 if (log.is_enabled()) {
1534 ResourceMark rm; 1556 if (log.is_enabled()) {
1557 ResourceMark rm;
>> (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.
>>
changed.
>> (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)
>>
Done, also add a check NULL for:
1125 if (ld._name != NULL) {
1126 ld._name->decrement_refcount();
1127 } Thanks Yumin
>> 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