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

[Roman Kennke]

On Thu, 8 Feb 2024 13:13:25 GMT, Stefan Karlsson <stefank at openjdk.org> wrote:

>> 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?

I am shortening it to 'self_fwd_*' instead, otherwise it all gets too wide, IMO. I hope that's ok?

> 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);

Good suggestion, I am doing that.

> 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) ).

Yes, I prefer that, too. Also, once the SerialGC gets its new (compressor) GC (see #17312), we can remove the 'marked' language altogether, because the bit-pattern no longer means 'marked' but only 'forwarded'. In-fact, we could even (finally?) rename the markWord to header (or a variant like objectHeader) because it would no longer be used for marking at all.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/17755#discussion_r1483064248
PR Review Comment: https://git.openjdk.org/jdk/pull/17755#discussion_r1483064658
PR Review Comment: https://git.openjdk.org/jdk/pull/17755#discussion_r1483068393