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

[Paul Sandoz]

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.

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.

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.
-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.)
-Update documentation for implementation notes and for `throws UnsupportedOperationException`.

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.

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

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