[master] RFR: Read class from object header [v3]

[Aleksey Shipilev]

On Wed, 28 Jul 2021 13:22:42 GMT, Roman Kennke <rkennke at openjdk.org> wrote:

>> This changes the Hotspot runtime to load the Klass* from the header instead of the dedicated Klass* word. The dedicated word is only still used for verification and for access by generated code (the former will eventually go away, the latter will be implemented separately).
>> 
>> Currently, this means we need to coordinate with the ObjectSynchronizer: when encountering a header that is a stack lock or a monitor, the header is displaced. Worse, if it is a stack-locked that is owned by a thread other than the calling thread, we must first inflate the lock to a full monitor. This is particularily bad for GCs. Luckily, most paths only do this at a safepoint, where we can actually safely access foreign stack locks and don't need to worry about inflation. Notably exception is concurrent marking by G1GC, which can cause inflation of locks, but it doesn't hurt very much.
>> 
>> It's really bad for Shenandoah and ZGC, though: when relocating objects, GC needs to know the object size of the from-space copy. However, this can cause inflation, and inflation creates new WeakHandle in the resulting monitor, and that would be initialized with a from-space copy, which is a no-go during evacuation/relocation.
>> 
>> That said, I have been told that work is under way to get rid of displaced headers altogether, which would neatly solve all those problems. I have no desire to make complicated workarounds for Shenandoah GC and ZGC. I disabled both in my own builds for now, and will implement them as soon as the monitor changes arrive.
>> 
>> In a couple of places in GC we need to access the header carefully: when concurrently forwarding (by parallel GC threads), we need to ensure we access the Klass* from an unforwarded header, and must also ensure to avoid re-loading the Klass* once we have the good header (that is why so many asserts have been removed - they would potentially re-load the Klass* from a header that may now be forwarded).
>> 
>> Testing:
>>  - [x] tier1
>>  - [x] tier2
>>  - [x] hotspot_gc
>> (all without Shenandoah and ZGC, see above)
>
> Roman Kennke has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Add missing new markWord.inline.hpp

All right, it looks fine for the experimental code. A few questions/comments:

src/hotspot/share/gc/shared/preservedMarks.inline.hpp line 58:

> 56:     header = header.displaced_mark_helper();
> 57:   }
> 58:   narrowKlass nklass = header.narrow_klass();

This assumes `UseCompressedClassPointers` is `true`, right? Needs to be asserted?

src/hotspot/share/oops/markWord.inline.hpp line 54:

> 52:   if (mrk.has_displaced_mark_helper()) {
> 53:     mrk = mrk.displaced_mark_helper();
> 54:   }

Suggestion:

  markWord m = *this;
  if (m.has_displaced_mark_helper()) {
    m = m.displaced_mark_helper();
  }

src/hotspot/share/oops/oop.hpp line 80:

> 78:   inline Klass* klass_or_null_acquire() const;
> 79: 
> 80:   narrowKlass narrow_klass() const { return _metadata._compressed_klass; }

Maybe we should rename this to e.g. `narrow_klass_legacy` to avoid accidentally using it? AFAIU, this is for legacy code paths?

src/hotspot/share/oops/oop.inline.hpp line 117:

> 115:   markWord header = mark();
> 116:   if (!header.is_neutral()) {
> 117:     header =ObjectSynchronizer::stable_mark(cast_to_oop(this));

Suggestion:

    header = ObjectSynchronizer::stable_mark(cast_to_oop(this));

src/hotspot/share/runtime/synchronizer.cpp line 755:

> 753:     // This is a stack lock owned by the calling thread so fetch the
> 754:     // displaced markWord from the BasicLock on the stack.
> 755:     mark = mark.displaced_mark_helper();

Do we have to check `has_displaced_mark_helper` before accessing?

src/hotspot/share/runtime/synchronizer.cpp line 763:

> 761:     assert(mark.is_neutral(), "invariant: header=" INTPTR_FORMAT, mark.value());
> 762:     assert(!mark.is_marked(), "no forwarded objects here");
> 763:     return mark;

Style: seems like `return mark` can be moved at the end of the method.

-------------

Marked as reviewed by shade (Committer).

PR: https://git.openjdk.java.net/lilliput/pull/12