RFR: 8305898: Alternative self-forwarding mechanism [v2]

[Stefan Karlsson]

On Thu, 8 Feb 2024 12:42:22 GMT, Roman Kennke <rkennke at openjdk.org> wrote:

>> Currently, the Serial, Parallel and G1 GCs store a pointer to self into object headers to indicate promotion failure. This is problematic for compact object headers ([JDK-8294992](https://bugs.openjdk.org/browse/JDK-8294992)) because it would (temporarily) over-write the crucial class information, which we need for heap parsing. I would like to propose an alternative: use the currently unused 3rd header bit (previously biased-locking bit) to indicate that an object is 'self-forwarded'. That preserves the crucial class information in the upper bits of the header until the full header gets restored.
>> 
>> This is a trimmed-down/simplified version of the original proposal #13779:
>>  - It doesn't use/introduce any flags and avoids the associated branching.
>>  - It doesn't (need to) deal with displaced headers. (Current code would preserve header if necessary, Lilliput code would not use displaced headers and set the 3rd bit directly in existing header.)
>> 
>> Testing:
>>  - [x] hotspot_gc
>>  - [x] tier1
>>  - [x] tier2
>
> Roman Kennke has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Align fake-oop in gtest

src/hotspot/share/oops/markWord.hpp line 119:

> 117:   static const uintptr_t lock_mask_in_place       = lock_mask << lock_shift;
> 118:   static const uintptr_t self_forwarded_mask      = right_n_bits(self_forwarded_bits);
> 119:   static const uintptr_t self_forwarded_mask_in_place = self_forwarded_mask << self_forwarded_shift;

Could you readjust the rest of the assignment so that we keep these nice and tidy?

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

> 292: }
> 293: 
> 294: oop oopDesc::forward_to_self_atomic(markWord compare, atomic_memory_order order) {

The new `forward_to_self_atomic` shares most of the code with `forward_to_atomic`. Would it make sense to extract the shared part into a helper? Maybe a:


oop oopDesc::cas_set_forwardee(markWord new_mark, markWord compare, atomic_memory_order order) {
  markWord old_mark = cas_set_mark(new_mark, compare, order);
  if (old_mark == compare) {
    return nullptr;
  } else {
    assert(old_mark.is_marked(), "must be marked here");
    return forwardee(old_mark);
  }
}


and then:

oop oopDesc::forward_to_atomic(oop to, markWord compare, atomic_memory_order order) {
  markWord new_mark = markWord::encode_pointer_as_mark(to);
  assert(forwardee(new_mark) == to, "encoding must be reversible");
  
  return cas_set_forwardee(new_mark, compare, order);
}

oop oopDesc::forward_to_self_atomic(markWord compare, atomic_memory_order order) {
  markWord new_mark = markWord::prototype().set_self_forwarded();
  assert(forwardee(new_mark) == cast_to_oop(this), "encoding must be reversible");
  
  return cas_set_forwardee(new_mark, compare, order);

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

> 304: }
> 305: oop oopDesc::forwardee(markWord mark) const {
> 306:   assert(mark.is_marked(), "only decode when actually forwarded");

I think it would be nice if this assert could instead be written as:

assert(mark.is_forwarded(), "only decode when actually forwarded");


Would people be opposed to adding a `markWord::is_forwarded` function? We already have one for oopDesc:

bool oopDesc::is_forwarded() const {
  // The extra heap check is needed since the obj might be locked, in which case the
  // mark would point to a stack location and have the sentinel bit cleared
  return mark().is_marked();
}

(which also could use `is_forwarded` instead of `is_marked`. (BTW, the comment is old and should probably be removed) ).

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

PR Review Comment: https://git.openjdk.org/jdk/pull/17755#discussion_r1482958735
PR Review Comment: https://git.openjdk.org/jdk/pull/17755#discussion_r1482973817
PR Review Comment: https://git.openjdk.org/jdk/pull/17755#discussion_r1482979620