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 20:00:03 UTC 2020


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.

  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