On Wed, 22 Jan 2025 15:01:03 GMT, Matthias Ernst <d...@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@25-ea/Native
>> Method)
>> ...
>> at
>> jdk.internal.foreign.abi.SharedUtils.newBoundedArena(java.base@25-ea/SharedUtils.java:386)
>> at
>> jdk.internal.foreign.abi.DowncallStub/0x000001f001084c00.invoke(java.base@25-ea/Unknown
>> Source)
>> ...
>> at
>> java.lang.invoke.Invokers$Holder.invokeExact_MT(java.base@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
