On Thu, 20 Jul 2023 09:25:31 GMT, Maurizio Cimadamore <mcimadam...@openjdk.org> wrote:
>> Glavo has updated the pull request with a new target base due to a merge or >> a rebase. The incremental webrev excludes the unrelated changes brought in >> by the merge/rebase. The pull request contains 15 additional commits since >> the last revision: >> >> - Merge branch 'openjdk:master' into unsafe >> - add 8310843 to @bug >> - Merge branch 'openjdk:master' into unsafe >> - Merge branch 'openjdk:master' into unsafe >> - delete incorrect comments >> - delete extraneous whitespace >> - add javadoc >> - delete extraneous whitespace >> - fix test >> - update tests >> - ... and 5 more: https://git.openjdk.org/jdk/compare/3f1edf8c...cb56e736 > > 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); ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/14636#discussion_r1269204424
