On Thu, 20 Jul 2023 09:33:15 GMT, Maurizio Cimadamore <[email protected]>
wrote:
>> src/java.base/share/classes/jdk/internal/util/ByteArray.java line 54:
>>
>>> 52:
>>> 53: @ForceInline
>>> 54: static long arrayOffset(byte[] array, int typeBytes, int offset) {
>>
>> IMHO, this is the really interesting thing that this class does - e.g. it
>> introduces a way to translate a (logical) offset into a byte array into a
>> physical offset that can be used for unsafe. After you have an helper method
>> like this, it seems like the client can just do what it wants by using
>> Unsafe directly (which would remove the need for having this class) ? Was
>> some experiment of that kind done (e.g. replacing usage of ByteArray with
>> Unsafe + helpers) - or does it lead to code that is too cumbersome on the
>> client?
>>
>> Also, were ByteBuffers considered as an alternative? (I'm not suggesting
>> MemorySegment as those depend on VarHandle again, but a heap ByteBuffer is
>> just a thin wrapper around an array which uses Unsafe). ByteBuffer will have
>> a bound check, but so does your code (which call checkIndex). I believe
>> that, at least in hot code, wrapping a ByteBuffer around a byte array should
>> be routinely scalarized, as there's no control flow inside these little
>> methods.
>
> Actually, a byte buffer is big endian, so some extra code would be required.
> But maybe that's another helper function:
>
>
> @ForceInline
> ByteBuffer asBuffer(byte[] array) { return
> ByteBuffer.wrap(array).order(ByteOrder.nativeOrder()); }
>
>
> And then replace:
>
>
> ByteArray.getChar(array, 42)
>
> With
>
> asBuffer(array).getChar(42);
Also... in a lot of cases where ByteArray is used (DataXYZStream,
ObjectXYZStream) the array being used is a field in the class. So the byte
buffer creation can definitively be amortized (or the code changed to work on
buffers instead of arrays).
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/14636#discussion_r1269210990
