RFR: 8287788: Implement a better allocator for downcalls [v13]

[Jorn Vernee]

On Wed, 22 Jan 2025 15:01:03 GMT, Matthias Ernst <duke at openjdk.org> wrote:

>> Certain signatures for foreign function calls (e.g. HVA return by value) require allocation of an intermediate buffer to adapt the FFM's to the native stub's calling convention. In the current implementation, this buffer is malloced and freed on every FFM invocation, a non-negligible overhead.
>> 
>> Sample stack trace:
>> 
>>    java.lang.Thread.State: RUNNABLE
>> 	at jdk.internal.misc.Unsafe.allocateMemory0(java.base at 25-ea/Native Method)
>> ...
>> 	at jdk.internal.foreign.abi.SharedUtils.newBoundedArena(java.base at 25-ea/SharedUtils.java:386)
>> 	at jdk.internal.foreign.abi.DowncallStub/0x000001f001084c00.invoke(java.base at 25-ea/Unknown Source)
>> ...
>> 	at java.lang.invoke.Invokers$Holder.invokeExact_MT(java.base at 25-ea/Invokers$Holder)
>> 
>> 
>> To alleviate this, this PR implements a per carrier-thread stacked allocator.
>> 
>> Performance (MBA M3):
>> 
>> 
>> Before:
>> Benchmark                    Mode  Cnt   Score   Error  Units
>> CallOverheadByValue.byPtr    avgt   10   3.333 ? 0.152  ns/op
>> CallOverheadByValue.byValue  avgt   10  33.892 ? 0.034  ns/op
>> 
>> After:
>> Benchmark                    Mode  Cnt  Score   Error  Units
>> CallOverheadByValue.byPtr    avgt   30  3.311 ? 0.034  ns/op
>> CallOverheadByValue.byValue  avgt   30  6.143 ? 0.053  ns/op
>> 
>> 
>> `-prof gc` also shows that the new call path is fully scalar-replaced vs 160 byte/call before.
>
> Matthias Ernst has updated the pull request incrementally with one additional commit since the last revision:
> 
>   an attempt at a stress test

src/java.base/share/classes/jdk/internal/foreign/abi/BufferStack.java line 97:

> 95:             private void assertOrder() {
> 96:                 if (tos != stack.currentOffset())
> 97:                     throw new IllegalStateException("Out of order access: frame not TOS");

I'd prefer not using an abbreviation here. (probably for the variable name as well)

Suggestion:

                    throw new IllegalStateException("Out of order access: frame not top-of-stack");

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

> 41: import jdk.internal.foreign.abi.x64.sysv.SysVx64Linker;
> 42: import jdk.internal.foreign.abi.x64.windows.Windowsx64Linker;
> 43: import jdk.internal.misc.CarrierThreadLocal;

Spurious import?
Suggestion:

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

> 384:     }
> 385: 
> 386:     private static final BufferStack LINKER_STACK = new BufferStack(256);

I think we want to make the size configurable. That would also make it easier to test with different values
Suggestion:

    private static final int LINKER_STACK_SIZE = Integer.getInteger("jdk.internal.foreign.LINKER_STACK_SIZE", 256);
    private static final BufferStack LINKER_STACK = new BufferStack(LINKER_STACK_SIZE);

test/jdk/java/foreign/TestBufferStack.java line 9:

> 7:  * published by the Free Software Foundation.  Oracle designates this
> 8:  * particular file as subject to the "Classpath" exception as provided
> 9:  * by Oracle in the LICENSE file that accompanied this code.

This is the wrong copyright header. Note that tests have a different header which does not include the class path exception.

test/micro/org/openjdk/bench/java/lang/foreign/CallOverheadByValue.java line 2:

> 1: /*
> 2:  * Copyright (c) 2021, 2024, Oracle and/or its affiliates. All rights reserved.

Suggestion:

 * Copyright (c) 2025, Oracle and/or its affiliates. All rights reserved.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/23142#discussion_r1925670249
PR Review Comment: https://git.openjdk.org/jdk/pull/23142#discussion_r1925665999
PR Review Comment: https://git.openjdk.org/jdk/pull/23142#discussion_r1925668240
PR Review Comment: https://git.openjdk.org/jdk/pull/23142#discussion_r1925676168
PR Review Comment: https://git.openjdk.org/jdk/pull/23142#discussion_r1925669213