RFR: 8356126: Duplication handling and optimization of CaptureCallState [v7]

Jorn Vernee jvernee at openjdk.org
Tue May 6 17:30:15 UTC 2025


On Tue, 6 May 2025 15:51:54 GMT, Chen Liang <liach at 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.wensj at 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


More information about the core-libs-dev mailing list