On Fri, 6 Dec 2024 16:30:47 GMT, Quan Anh Mai <[email protected]> wrote:
> Hi,
>
> This patch improves the performance of a typical `Arena::allocate` in several
> ways:
>
> - Delay the creation of the NativeMemorySegmentImpl. This avoids the merge of
> the instance with the one obtained from the call in the uncommon path,
> increasing the chance the object being scalar replaced.
> - Split the allocation of over-aligned memory to a slow-path method.
> - Align the memory to 8 bytes, allowing faster zeroing.
> - Use a dedicated method to zero the just-allocated native memory, reduce
> code size and make it more straightforward.
> - Make `VM.pageAlignDirectMemory` a `Boolean` instead of a `boolean` so that
> `false` value can be constant folded.
>
> Please take a look and leave your reviews, thanks a lot.
src/java.base/share/classes/jdk/internal/foreign/ArenaImpl.java line 53:
> 51: Utils.checkAllocationSizeAndAlign(byteSize, byteAlignment);
> 52: long address = SegmentFactories.allocateNative(byteSize,
> byteAlignment, session, shouldReserveMemory, false);
> 53: return new NativeMemorySegmentImpl(address, byteSize, false,
> session);
Could you move this constructor call (and the one below) to `allocateNative`?
All segment construction calls are currently in `SegmentFactories` as a measure
to avoid bootstrap cycles, which we had problems with in the past.
src/java.base/share/classes/jdk/internal/foreign/SegmentFactories.java line 213:
> 211:
> 212: @DontInline
> 213: private static long allocateNativeOveraligned(long byteSize, long
> byteAlignment, MemorySessionImpl sessionImpl,
This would always make the session escape in the case of over aligned
allocation. Maybe it's possible to move the call to `addCleanupIfFail` to
`allocateNative` instead? (that seems to be the only use of `sessionImpl` here).
test/micro/org/openjdk/bench/java/lang/foreign/AllocTest.java line 87:
> 85: }
> 86:
> 87: public static class CallocArena implements Arena {
The following changes seem to be here to support 32-bit platforms? Could you
please do that in a separate PR?
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/22610#discussion_r1873822362
PR Review Comment: https://git.openjdk.org/jdk/pull/22610#discussion_r1873826657
PR Review Comment: https://git.openjdk.org/jdk/pull/22610#discussion_r1873831139
