[foreign-memaccess+abi] RFR: Improve performance of Arena::allocateFrom [v2]

[Jorn Vernee]

On Thu, 3 Aug 2023 21:29:29 GMT, Maurizio Cimadamore <mcimadamore at openjdk.org> wrote:

>> This patch improves the performance of allocation of a standard confined/shared arenas in two steps:
>> 
>> * first, it special cases the allocation methods in SegmentAllocator to detect the case where the SegmentAllocator implementation is the internal Arena implementation. In such case, all the `allocateFrom` methods attempt an allocation request which does not perform memory zeroing (as the contents are going to be overwritten anyway).
>> * second, it minimizes the overhead associated with reserving/unreserving memory. More specifically, it only calls Bits::reserveMemory/unreserveMemory when allocating from an automatic arena.
>> 
>> Implementation-wise, this is done by having an internal arena implementation class (`ArenaImpl`) which is the implementation returned by the various arena factories. This class will have methods to allocate zeroed memory and non-zeroed memory. In order to avoid duplication of the various allocation routines, I instead re-routed the SegmentAllocator methods to a private allocation implementation which sees if we're `ArenaImpl` and if so calls the implementation that has better knowledge. Alternatively I could have overridden all `allocateFrom` methods from `SegmentAllocator` but that would have required some duplication.
>> 
>> One caveat: a custom arena does NOT inherit this special behavior. That is, it is the responsibility of the custom arena to define "shortcut" for the `allocateFrom` methods. The only way to avoid that would be to have zeroing as an explicit boolean parameter in the allocation methods, but that's not very safe, as it is now up to client to decide if they want zeroing or not. That said, this is only an issue for custom arenas, and we can assume that a client that wants a specialized arena behavior can handle overriding a bunch of methods via delegation (in case they care).
>
> Maurizio Cimadamore has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Fix segment allocator test

src/java.base/share/classes/jdk/internal/foreign/abi/SharedUtils.java line 95:

> 93:         public MemorySegment allocate(long byteSize, long byteAlignment) {
> 94:             throw new UnsupportedOperationException();
> 95:         }

Nice, so making the allocate method abstract caught this bug.

test/jdk/java/foreign/TestSegmentAllocators.java line 160:

> 158:     public void testBadArenaNullReturn() {
> 159:         try (Arena arena = Arena.ofConfined()) {
> 160:              arena.allocate(Long.MAX_VALUE, 2);

This change seems spurious? (I count 5 space indentation now).

test/micro/org/openjdk/bench/java/lang/foreign/AllocFromTest.java line 24:

> 22:  * or visit www.oracle.com if you need additional information or have any
> 23:  * questions.
> 24:  */

Wrong copyright header (year and CE)

test/micro/org/openjdk/bench/java/lang/foreign/AllocFromTest.java line 67:

> 65:     public void setup() {
> 66:         arr = new byte[size];
> 67:         Random random = new Random();

Please use an explicit seed here (e.g. `0`), so that the data is always the same between benchmark runs.

test/micro/org/openjdk/bench/java/lang/foreign/AllocFromTest.java line 84:

> 82:         MemorySegment segment = arena.allocateFrom(ValueLayout.JAVA_BYTE, arr);
> 83:         arena.close();
> 84:         return segment.address();

Why not return the `segment` here as well? (like in `alloc`). Seems like an unfair comparison?

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

PR Review Comment: https://git.openjdk.org/panama-foreign/pull/855#discussion_r1284406572
PR Review Comment: https://git.openjdk.org/panama-foreign/pull/855#discussion_r1284406991
PR Review Comment: https://git.openjdk.org/panama-foreign/pull/855#discussion_r1284407663
PR Review Comment: https://git.openjdk.org/panama-foreign/pull/855#discussion_r1284408662
PR Review Comment: https://git.openjdk.org/panama-foreign/pull/855#discussion_r1284410337