RFR: 8275527: Separate/refactor forward pointer access

[Stefan Karlsson]

On Thu, 14 Oct 2021 16:37:02 GMT, Roman Kennke <rkennke at openjdk.org> wrote:

> Accessing the forward pointer overlay of object headers is currently done in a somewhat crude way. oopDesc::forwardee() and oopeDesc::is_forwarded() both load the header-word. This seems kinda bad, because they are most often used in conjunction, e.g.:
> fwd = obj->forwarded() ? obj->forwardee() : promote_obj();
> 
> or similar constructs.
> 
> Also, in some places, the forwardee is accessed in a more direct fashion using markWord::decode_pointer(), or as HeapWord*, sometimes is_forwarded() is determined by forwardee() == NULL checks (and crude overrides to prepare non-forwarded headers to return NULL in such checks, see g1Full* source files).
> 
> I propose to extract and refactor forward pointer access in a way to better support above pattern. In-fact I originally wrote this with performance-enhancement in mind (see oopForwarding.hpp header for more details), but I could not show any actual performance improvements, so let's consider this purely on cleanliness and separation-of-concerns basis.
> 
> Testing:
>  - [x] tier
>  - [x] tier2
>  - [x] hotspot_gc

I don't think we should make the code more complicated by using the micro optimized version unless we can measure the benefits. IMHO, it is harder to read and it makes it more difficult to know if the read-once of was done for correctness purposes or not. I understand that that other reviewers might disagree with my view, and if that's the case, then I'll back off.

Some concrete feedback on the current patch:

I don't like that we store the oop in the OopForwarding. That oop gets snuck into functions via the fwd variable. That makes it less obvious that the functions depend on the old oop. Code like the following becomes more obscure, when you don't immediately see that it operates on the original (old) object:

  const oop forward_ptr = fwd.forward_to_atomic(obj, memory_order_relaxed);
  if (forward_ptr == NULL) {


I wonder if it wouldn't make sense to make forward_to_atomic static, just like the forward_to is static? That way we wouldn't have to store _obj in OopForwarding, and I think the code would be more decoupled and easier to read.

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

PR: https://git.openjdk.java.net/jdk/pull/5955