RFR: 8139457: Array bases are aligned at HeapWord granularity [v50]

[Albert Mingkun Yang]

On Wed, 16 Aug 2023 20:47:11 GMT, Roman Kennke <rkennke at openjdk.org> wrote:

>>> I did that in an earlier version of this PR and it looked quite messy ...
>> 
>> I just checked the first commit and the `MacroAssembler` part matches what I have in mind. I believe that diff being messy is due to "Calculations of min and max array and tlab sizes".
>> 
>>> so we would have to check if the body is aligned or not, and if not, add an extra store
>> 
>> The store, as I understand it, belongs to the body (payload) semantically. Mixing stores btw `initialize_header` and `initialize_body` is super confusing, IMO.
>> 
>>> This is also consistent with how we store the klass-gap in instance objects there.
>> 
>> klass-gap is part of the header, so it makes sense to be in `initialize_header`.
>
>> > I did that in an earlier version of this PR and it looked quite messy ...
>> 
>> I just checked the first commit and the `MacroAssembler` part matches what I have in mind. I believe that diff being messy is due to "Calculations of min and max array and tlab sizes".
> 
> Yes, that is another part of it. I can re-instate the parts where we initialize the leading 4 bytes in the body, if agree that this is the way to go.
>  
>> > so we would have to check if the body is aligned or not, and if not, add an extra store
>> 
>> The store, as I understand it, belongs to the body (payload) semantically. Mixing stores btw `initialize_header` and `initialize_body` is super confusing, IMO.
> 
> 
> Yes, I understand that.
>  
>> > This is also consistent with how we store the klass-gap in instance objects there.
>> 
>> klass-gap is part of the header, so it makes sense to be in `initialize_header`.
> 
> No, klass-gap really is not part of the header. For non-array objects, this gap is where the first bunch of fields go, it really is payload. So, arguably, we should move the clearing of the gap into initialize_body(), too - but not as part of this PR.

There is sth odd about `instanceOopDesc` that the header/payload reported by public API `header_size` and `base_offset_in_bytes` can be overlapping, shown picturally:


|<-header->|....     |
|        |<-payload->|


Essentially, `header-size * HeapWordSize != base-offset-in-bytes`, blurring the boundary btw header and payload.

Possibly, it's intentional to overapproximate "header" and underapproximate "payload", because some context dependent constraints prevent `header_size` from being klass specific, i.e. having knowledge to the corresponding fields (layout). Otherwise, one should avoid declaring the existence of "klass-gap" then filling it up using some fields from the object. A klass-gap is there if and only if the first field has special alignment requirement.

> No, klass-gap really is not part of the header. For non-array objects, this gap is where the first bunch of fields go, it really is payload.

I can see where that interpretation comes from since `base_offset_in_bytes == klass_gap_offset_in_bytes` when `UseCompressedClassPointers == true` and base-offset denotes the start of payload.

It's not part of this diff -- in `C1_MacroAssembler::initialize_header`, can `movl(Address(obj, oopDesc::klass_offset_in_bytes()), t1);` use `movptr` like its 32bit counterpart?

Since the klass-layout-info is unavailable in this context, including this "potential" gap as part of the header is kind of overapproximation.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/11044#discussion_r1297170883