On Wed, 26 Feb 2025 05:00:09 GMT, Chen Liang <li...@openjdk.org> wrote:
>> As we advance, converting older JDK code to use the relatively new FFM API
>> requires system calls that can provide `errno` and the likes to explicitly
>> allocate a `MemorySegment` to capture potential error states. This can lead
>> to negative performance implications if not designed carefully and also
>> introduces unnecessary code complexity.
>>
>> Hence, this PR proposes adding a JDK internal method handle adapter that can
>> be used to handle system calls with `errno`, `GetLastError`, and
>> `WSAGetLastError`.
>>
>> It relies on an efficient carrier-thread-local cache of memory regions to
>> allide allocations.
>>
>>
>> Here are some benchmarks that ran on a platform thread and virtual threads
>> respectively (M1 Mac):
>>
>>
>> Benchmark Mode Cnt Score
>> Error Units
>> CaptureStateUtilBench.OfVirtual.adaptedSysCallFail avgt 30 24.330
>> ? 0.820 ns/op
>> CaptureStateUtilBench.OfVirtual.adaptedSysCallSuccess avgt 30 8.257
>> ? 0.117 ns/op
>> CaptureStateUtilBench.OfVirtual.explicitAllocationFail avgt 30 41.415
>> ? 1.013 ns/op
>> CaptureStateUtilBench.OfVirtual.explicitAllocationSuccess avgt 30 21.720
>> ? 0.463 ns/op
>> CaptureStateUtilBench.OfVirtual.tlAllocationFail avgt 30 23.636
>> ? 0.182 ns/op
>> CaptureStateUtilBench.OfVirtual.tlAllocationSuccess avgt 30 8.234
>> ? 0.156 ns/op
>> CaptureStateUtilBench.adaptedSysCallFail avgt 30 23.918
>> ? 0.487 ns/op
>> CaptureStateUtilBench.adaptedSysCallSuccess avgt 30 4.946
>> ? 0.089 ns/op
>> CaptureStateUtilBench.explicitAllocationFail avgt 30 42.280
>> ? 1.128 ns/op
>> CaptureStateUtilBench.explicitAllocationSuccess avgt 30 21.809
>> ? 0.413 ns/op
>> CaptureStateUtilBench.tlAllocationFail avgt 30 24.422
>> ? 0.673 ns/op
>> CaptureStateUtilBench.tlAllocationSuccess avgt 30 5.182
>> ? 0.152 ns/op
>>
>>
>> Adapted system call:
>>
>> return (int) ADAPTED_HANDLE.invoke(0, 0); // Uses a MH-internal pool
>> ```
>> Explicit allocation:
>>
>> try (var arena = Arena.ofConfined()) {
>> return (int) HANDLE.invoke(arena.allocate(4), 0, 0);
>> }
>> ```
>> Thread Local allocation:
>>
>> try (var arena = POOLS.take()) {
>> return (int) HANDLE.invoke(arena.allocate(4), 0, 0); // Uses a
>> manually specified pool
>> }
>> ```
>> The adapted system call exhibits a ~4x performance improvement ove...
>
> src/java.base/share/classes/jdk/internal/foreign/CaptureStateUtil.java line
> 102:
>
>> 100: // A key that holds both the `returnType` and the `stateName`
>> needed to look up a
>> 101: // specific "basic handle" in the `BASIC_HANDLE_CACHE`.
>> 102: // returnType E {int.class | long.class}
>
> I think using `∈` or `\in` instead of `E` would be more clear.
I didn't know about `∈` so thanks for this piece of information @liach.
However, in `//` comments it might be better to just type "in".
> src/java.base/share/classes/jdk/internal/foreign/CaptureStateUtil.java line
> 211:
>
>> 209: // This is equivalent to:
>> 210: // computeIfAbsent(basicKey,
>> CaptureStateUtil::basicHandleFor);
>> 211: .computeIfAbsent(basicKey, new Function<>() {
>
> I recommend a local record and capture the record instance in a member static
> final field. This code creates a function on every call. Also might be of
> interest whether we should use get + putIfAbsent or computeIfAbsent, as CHM
> has some bug that makes cIA slower than get for certain access patterns.
Performance is not critical to this method so I would prioritize
maintainability over performance here.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/23765#discussion_r1971271580
PR Review Comment: https://git.openjdk.org/jdk/pull/23765#discussion_r1971267260
