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

[Roman Kennke]

On Wed, 13 Sep 2023 14:14:43 GMT, Roman Kennke <rkennke at openjdk.org> wrote:

>> There's gtest a failure in the GHA run:
>> 
>>  [ RUN      ] arrayOopDesc.double_vm
>> /home/runner/work/jdk/jdk/test/hotspot/gtest/oops/test_arrayOop.cpp:51: Failure
>> check_max_length_overflow(T_DOUBLE) evaluates to false, where
>> T_DOUBLE evaluates to 
>> 
>> [  FAILED  ] arrayOopDesc.double_vm (0 ms)
>> [ RUN      ] arrayOopDesc.byte_vm
>> [       OK ] arrayOopDesc.byte_vm (0 ms)
>> [ RUN      ] arrayOopDesc.short_vm
>> [       OK ] arrayOopDesc.short_vm (0 ms)
>> [ RUN      ] arrayOopDesc.int_vm
>> [       OK ] arrayOopDesc.int_vm (0 ms)
>> [ RUN      ] arrayOopDesc.long_vm
>> /home/runner/work/jdk/jdk/test/hotspot/gtest/oops/test_arrayOop.cpp:67: Failure
>> check_max_length_overflow(T_LONG) evaluates to false, where
>> T_LONG evaluates to 
>> 
>> 
>> [  FAILED  ] arrayOopDesc.long_vm (0 ms)
>
>> There's gtest a failure in the GHA run:
>> 
>> ```
>>  [ RUN      ] arrayOopDesc.double_vm
>> /home/runner/work/jdk/jdk/test/hotspot/gtest/oops/test_arrayOop.cpp:51: Failure
>> check_max_length_overflow(T_DOUBLE) evaluates to false, where
>> T_DOUBLE evaluates to �
>> 
>> [  FAILED  ] arrayOopDesc.double_vm (0 ms)
>> [ RUN      ] arrayOopDesc.byte_vm
>> [       OK ] arrayOopDesc.byte_vm (0 ms)
>> [ RUN      ] arrayOopDesc.short_vm
>> [       OK ] arrayOopDesc.short_vm (0 ms)
>> [ RUN      ] arrayOopDesc.int_vm
>> [       OK ] arrayOopDesc.int_vm (0 ms)
>> [ RUN      ] arrayOopDesc.long_vm
>> /home/runner/work/jdk/jdk/test/hotspot/gtest/oops/test_arrayOop.cpp:67: Failure
>> check_max_length_overflow(T_LONG) evaluates to false, where
>> T_LONG evaluates to �
>> 
>> [  FAILED  ] arrayOopDesc.long_vm (0 ms)
>> ```
> 
> Aww, this max_array_length() method and 32bit builds. :-/
> We should re-write this method altogether and special-case it for !_LP64 and maybe simply make it a switch on the incoming type, with hard-coded values. This might be easier to understand than getting the logic absolutely right. Also, with this change, and even more so with upcoming Lilliput changes, this method is a little too conservative and we could offer somewhat increased array lengths. Alternatively, we could do what the comments suggests and fix up all the uses of the method to use sensible types (size_t?) and make it simple and obvious.

> @rkennke apologies for the delay. I spent some time yesterday pondering what to do.
> 
> Since Lilliput will change array objects to be aligned on < 8 byte boundaries, then I don’t think we should in anyway rely on stability say at 4 byte boundaries for `byte[]`. We have to generally assume it could further reduce down to a smaller value e.g., 2 or even 1.

My current intention is to maintain stability at 4 byte boundaries. (In-fact, when object header are down to 4 bytes, and we still use 4 bytes for length, we will be back to 8-byte boundary for arrays.) I am not sure we can realistically reduce header sizes even more, tbh.
 
> In that respect I generally recommend for Lilliput we do the following (some of which is perhaps broader than this issue):
> 
> 1. Unsafe
>    -Document limitations e.g., in JEP and/or release note.

Ok.

> 2. `ByteBuffer::alignedSlice` and `ByteBuffer::alignmentOffset`
>    -Deprecate, not for removal, explaining limitations and referring to use of direct buffers, native memory segments, or heap memory segments covering `long[]` arrays.

Why should we deprecate those methods? See also next point.

>    -Update implementations to replace hard coded unit size of 8 to a runtime query for `byte[]`. Can we use `Unsafe.arrayBaseOffset`? (This is the only recommended implementation update.)

I believe that ByteBuffer et al already do use Unsafe.arrayBaseOffset(). I noted elsewhere that all relevant Buffer and VarHandles tests which test alignmentOffset() and alignedSlice() are passing with this change, and also with the greater Lilliput updates, because they already do the right thing, afaict.

>    -Update documentation for implementation notes and for `throws UnsupportedOperationException`.

Ok.

> 3. `MethodHandles::byteArrayViewVarHandle` and `MethodHandles::byteBufferViewVarHandle`
>    -Deprecate, not for removal, explaining limitations and referring to use of direct buffers, native memory segments, or heap memory segments covering `long[]` arrays.

OK, but see above.

BTW, another compatibility-related issue might be JVMCI/Graal. Graal JIT would need some updates for the changed object/array layout.

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

PR Comment: https://git.openjdk.org/jdk/pull/11044#issuecomment-1777559251