-
Notifications
You must be signed in to change notification settings - Fork 5.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
8305895: Implementation: JEP 450: Compact Object Headers (Experimental) #13844
Conversation
👋 Welcome back rkennke! A progress list of the required criteria for merging this PR into |
|
Webrevs
|
Reviewed-by: eosterlund, kvn
Reviewed-by: prr
I'm not sure if this is trivial or significant, but if you limit the class pointer to 30 bit, and use the upper 2 bits for locking, then you can obtain the class pointer in less instructions:
This exploits the fact that the most significant bit represents a negative number, so it clears the unrelated bit and checks for valid header at the same time, the sequence is only 2 instructions long after macro fusion, compared to the current value of 3. This also allows quick class comparisons against constants, assuming that most instance is in unlock state, the comparison when equality is likely can be done:
This can be matched on an Thanks. |
Reviewed-by: rriggs
Reviewed-by: asotona
These are great suggestions! I would shy away from doing it in this PR, though, because this also affects the locking subsystem and would cause quite intrusive changes and invalidate all the testing that we've done. Let's consider this in the Lilliput project and upstream the optimization separately, ok? Thanks! |
Reviewed-by: kvn, chagedorn, thartmann
This seems great as an intermediary step toward a 32 bits header. |
…ts (mainline) Reviewed-by: jvernee
…ed: IllegalStateException: Already closed Reviewed-by: jvernee
…broken by JDK-8301995 Reviewed-by: dnsimon, never, kvn
Reviewed-by: mcimadamore
Reviewed-by: shade, coleenp, tschatzl
…or SSLEngineTemplate Reviewed-by: xuelei
Reviewed-by: psandoz, tvaleev, rriggs
Reviewed-by: kizune
…ore checking exceptions Reviewed-by: dholmes, alanb
…gs as per RFC 5646 convention Reviewed-by: naoto, rriggs
…g unexpectedly with an exception Reviewed-by: kevinw, lmesnik
…n linux-x64 Reviewed-by: naoto, lmesnik
Reviewed-by: aph, adinn
Reviewed-by: dnguyen, honkar, psadhukhan
Reviewed-by: stefank
Reviewed-by: mbaesken, erikj
Reviewed-by: mdoerr, lucy
Reviewed-by: shade, gli, fyang
Reviewed-by: kbarrett, ayang
…re/ and /jpda that are used in serviceability/dcmd/framework tests Reviewed-by: coleenp, dholmes
…proc/meminfo Reviewed-by: stuefe, sgehwolf, rcastanedalo, dholmes
if (UseCompactObjectHeaders) { | ||
// The copy above is not atomic. Make sure we have seen the proper mark | ||
// and re-install it into the copy, so that Klass* is guaranteed to be correct. | ||
markWord mark = o->mark_acquire(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we need the acquire here, do we?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right. An atomic load would be sufficient, which is what oopDesc::mark() already does. I change those code paths to use just mark().
Reviewed-by: stefank, dholmes
…fails with ShouldNotReachHere Reviewed-by: jvernee
Reviewed-by: tschatzl, iwalulya
…est8168712 Reviewed-by: coleenp, thartmann
Reviewed-by: thartmann
…K-8140326 Reviewed-by: iwalulya, ayang
…ages Reviewed-by: alanb, rriggs
…ndly Reviewed-by: jpai
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have any comments on the compiler code or gc code, but some other comments and questions. Some of the LP64 preprocessor conditionals are inconsistent in the assembly code.
assert(!MacroAssembler::needs_explicit_null_check(oopDesc::mark_offset_in_bytes()), "must add explicit null check"); | ||
} else { | ||
assert(!MacroAssembler::needs_explicit_null_check(oopDesc::klass_offset_in_bytes()), "must add explicit null check"); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We put this check in Universe::genesis() so it's not needed here for one less conditional. Maybe that check should be this one instead of what we have there.
void MacroAssembler::load_nklass_compact(Register dst, Register src) { | ||
assert(UseCompactObjectHeaders, "expects UseCompactObjectHeaders"); | ||
|
||
if (!UseCompactObjectHeaders) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm confused, why is this conditional here if you asserted it before? I can't imagine this being an untested code path and you need this for safety. If so, this doesn't take CompressedKlassPointers into account. I think it would be better to remove it. If I'm reading this right. Maybe change this assert to a guarantee for testing if you think this is likely.
I see why this is. This is inconsistent with x86. You should fix this to match x86 and make it load_narrow_klass().
assert_different_registers(obj, klass, len); | ||
movptr(Address(obj, oopDesc::mark_offset_in_bytes()), checked_cast<int32_t>(markWord::prototype().value())); | ||
assert_different_registers(obj, klass, len, t1, t2); | ||
if (UseCompactObjectHeaders) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this be in _LP64 too like the code just above?
} | ||
|
||
if (len->is_valid()) { | ||
movl(Address(obj, arrayOopDesc::length_offset_in_bytes()), len); | ||
if (UseCompactObjectHeaders) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should also be in _LP64 and not have && !UseCompactObjectHeaders. You should restrict this to LP64 in this change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok I will put it in _LP64 (even though it is not strictly needed - UseCompactObjectHeaders is hard-wired constant false, so compiler will not include the code, I would expect), but why not check UseCompactObjectHeaders here? The new code is only sensible with compact headers.
src/hotspot/share/oops/klass.cpp
Outdated
#endif | ||
return prototype; | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like a useful change without UseCompactObjectHeaders as an enhancement and to remove some conditional code. Since we have storage in Klass for it anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why? This code used to be there with BiasedLocking, and has been removed. I've re-instated it for compact object headers, because the prototype mark for an object now depends on its Klass, but other than that, why would it be useful? The prototype would be just markWord::prototype().
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought not to waste a 64 bit field in Klass and to maybe eliminate some if (CompactObjectHeaders) use the one in Klass else use MarkWord::prototype(), just always use the one in Klass. Minimizing if (CompactObjectHeaders) would be a good thing. At any case, this isn't for this change, just an idea to try to use this field unconditionally. I recognize it from BiasedLocking.
@@ -156,7 +156,8 @@ ObjArrayKlass::ObjArrayKlass(int n, Klass* element_klass, Symbol* name) : ArrayK | |||
} | |||
|
|||
size_t ObjArrayKlass::oop_size(oop obj) const { | |||
assert(obj->is_objArray(), "must be object array"); | |||
// In this assert, we cannot safely access the Klass* with compact headers. | |||
assert(UseCompactObjectHeaders || obj->is_objArray(), "must be object array"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't there code that checks oop->is_objArray() before calling this? Would it return true when it's not an objArray?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, there is. We're a bit excessive with asserting the klass here. I tried to remain as close as possible with that, so I disabled it only for compact object headers.
return _metadata._klass; | ||
} | ||
} | ||
|
||
Klass* oopDesc::klass_or_null() const { | ||
if (UseCompressedClassPointers) { | ||
#ifdef _LP64 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like all these #ifdef _LP64 here. Maybe markWord.inline.hpp can be refactored to not require callers to have this conditional inclusion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem is with 32bits, in markWord, we only have 32bits in the header, and no place to stick in the Klass* in the upper 32 bits. That's why I put all those #ifdefs, there.
If you take a step back, you'll notice that compact object headers mostly aligns the layout headers of 64bit and 32bit JVMs. There would be a great opportunity here to consolidate all this code, make the whole header a union/struct/bitfield that looks the same both on 32bit and 64bit builds. But this conflicts with the current implementation where we want to be able to switch between compact and legacy header layout.
Also, going forward, we want to shrink the header even more to just 32bits, and still have it switchable with the old layout. Eventually all this stuff will be the same in 32bit and 64bit JVMs, but for the time being I think we need to keep it slightly messy to support the legacy layout.
} | ||
if (UseCompactObjectHeaders && !UseCompressedClassPointers) { | ||
FLAG_SET_DEFAULT(UseCompressedClassPointers, true); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make this a function like set_compact_object_headers_flags(), that checks for FLAG_IS_CMDLINE for the related options and give a warning for them too, and add a test.
…ted method paragraph Reviewed-by: jvernee
@rkennke Can you merge up with the GenerationalZGC changes because some of our test definitions need it. |
@rkennke Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. See OpenJDK Developers’ Guide for more information. |
I'm sorry, I think I butchered this PR while trying to merge latest upstream through all the dependent PRs. Let's continue the discussion the new PR #13961. I hope I haven't caused anything breakage (for some reason, this PR now shows as "Merged" which worries me. I believe the Skara bot did that. I wonder where it has been merged to.) |
This is the main body of the JEP 450: Compact Object Headers (Experimental).
Main changes:
Testing:
(+UseCompactObjectHeaders tests are run with the flag hard-patched into the build, to also catch @flagless tests, and to avoid mismatches with CDS - see above.)
Progress
Integration blocker
Issues
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/13844/head:pull/13844
$ git checkout pull/13844
Update a local copy of the PR:
$ git checkout pull/13844
$ git pull https://git.openjdk.org/jdk.git pull/13844/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 13844
View PR using the GUI difftool:
$ git pr show -t 13844
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/13844.diff
Webrev
Link to Webrev Comment