RFR: 8178349: Cache builtin class loader constraints to avoid re-initializing itable/vtable for shared classes
Yumin Qi
yumin.qi at oracle.com
Wed Apr 29 04:38:18 UTC 2020
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