On Tue, 6 May 2025 15:51:54 GMT, Chen Liang <li...@openjdk.org> wrote:
>> Credit to @lukellmann that the duplication arg handling in #24742 avoided
>> throwing exceptions but produced a wrong option. This patch fixes that and
>> removed stream usages in CaptureCallState to speed up bootstrap.
>>
>> Also, the previous patch affected the toString display of the option; I
>> added a unit test to ensure the option prints names that is user-friendly.
>>
>> Another thing I noted is `CapturableState` uses `OperatingSystem`; using
>> `valueOf` brings a performance overhead due to setups with reflection, so I
>> made this lazy. (The enum is thread safe, so we allow racy access to the
>> cache field)
>>
>> Testing: jdk/lang/foreign, tier 1-3 in progress.
>
> Chen Liang has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Update
> src/java.base/share/classes/jdk/internal/foreign/abi/CapturableState.java
>
> Co-authored-by: Shaojin Wen <shaojin.we...@alibaba-inc.com>
Looks good.
src/java.base/share/classes/jdk/internal/foreign/abi/CapturableState.java line
55:
> 53: } else {
> 54: supported = List.of(new CapturableState("errno", JAVA_INT, 1
> << 2));
> 55: }
Maybe just split the initialization of `LAYOUT` and `LOOKUP` across these 2
branches, instead of collecting everything into intermediate arrays. i.e.
Suggestion:
if (OperatingSystem.isWindows()) {
LAYOUT = MemoryLayout.structLayout(
JAVA_INT, // GetLastError
JAVA_INT, // WSAGetLastError
JAVA_INT // errno
);
LOOKUP = Map.of(
"GetLastError", 1 << 0,
"WSAGetLastError", 1 << 1,
"errno", 1 << 2
);
} else {
LAYOUT = MemoryLayout.structLayout(
JAVA_INT // errno
);
LOOKUP = Map.of(
"errno", 1 << 2
);
}
-------------
Marked as reviewed by jvernee (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/25025#pullrequestreview-2819101227
PR Review Comment: https://git.openjdk.org/jdk/pull/25025#discussion_r2075950579
