On Wed, 15 Nov 2023 22:46:03 GMT, Jorn Vernee <jver...@openjdk.org> wrote:
> See the JBS issue for an extended problem description.
>
> This patch changes the specification and implementation of
> `MethodHandles::byteArrayViewVarHandle`,
> `MethodHandles::byteBufferViewVarHandle`, `ByteBuffer::alignedSlice`, and
> `ByteBuffer::alignmentOffset` to weaken the guarantees they make about the
> alignment of Java array elements, in order to bring them in line with the
> guarantees made by an arbitrary JVM implementation (which makes no guarantees
> about array element alignment beyond them being aligned to their natural
> alignment).
>
> - `MethodHandles::byteArrayViewVarHandle`: we can not guarantee any alignment
> for the accesses. Which means we can only reliably support plain get and set
> access modes. The javadoc text explaining which other access modes are
> supported, or how to compute aligned offsets into the array is dropped, as it
> is not guaranteed to be correct on all JVM implementations. The
> implementation of the returned VarHandle is changed to throw an
> `UnsupportedOperationException` for the unsupported access modes, as mandated
> by the spec of `VarHandle` [1].
>
> - `MethodHandles::byteBufferViewVarHandle`: the implementation/spec is
> incorrect when accessing a heap buffer (wrapping a byte[]), for the same
> reasons as `byteArrayViewVarHandle`. The spec is changed to specify that when
> accessing a _heap buffer_, only plain get and set access modes are supported.
> The implementation of the returned var handle is changed to throw an
> `IllegalStateException` when an access is attempted on a heap buffer using an
> access mode other than plain get or set. Note that we don't throw an outright
> `UnsupportedOperationException` for this case, since whether the access modes
> are supported depends on the byte buffer instance being used.
>
> - `ByteBuffer::alignedSlice` and `ByteBuffer::alignmentOffset`: The former
> method depends directly on the latter for all its alignment computations. We
> change the implementation of the latter method to throw an
> `UnsupportedOperationException` for all unit sizes greater than 1, when the
> buffer is non-direct. This change is largely covered by the existing
> specification:
>
>
> * @throws UnsupportedOperationException
> * If the native platform does not guarantee stable alignment
> offset
> * values for the given unit size when managing the memory regions
> * of buffers of the same kind as this buffer (direct or
> * non-direct). For example, if garbage collection would result
> * in the mo...
src/java.base/share/classes/java/lang/invoke/MethodHandles.java line 4518:
> 4516: * Only plain {@linkplain VarHandle.AccessMode#GET get} and
> {@linkplain VarHandle.AccessMode#SET set}
> 4517: * access modes are supported by the returned var handle. For all
> other access modes, an
> 4518: * {@link UnsupportedOperationException} will be thrown.
I recommend adding an api note explaining that native memory segments, direct
byte buffers, or heap memory segments backed by long[] should be used if
support for other access modes are required.
src/java.base/share/classes/java/lang/invoke/MethodHandles.java line 4610:
> 4608: * {@link Double#doubleToRawLongBits}, respectively).
> 4609: * <p>
> 4610: * Access to heap byte buffers is always unaligned.
I recommend merging that sentence into the paragraph on heap byte buffers e.g.:
> For direct buffers, access of the bytes at an index is always misaligned. As
> a result only the plain...
src/java.base/share/classes/java/nio/X-Buffer.java.template line 2218:
> 2216: * @implNote
> 2217: * This implementation throws {@code UnsupportedOperationException}
> for
> 2218: * non-direct buffers when the given unit size is greater then
> {@code 1}.
This is no longer an implementation note, its now part of the specified API. So
i think we can simplify the text of the `@throws UOE ...` to just say:
@throws UOE if the buffer is non-direct and the unit size > 1
Same for the other method.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/16681#discussion_r1396097577
PR Review Comment: https://git.openjdk.org/jdk/pull/16681#discussion_r1396101675
PR Review Comment: https://git.openjdk.org/jdk/pull/16681#discussion_r1396109655
