RFR: 8356126: Revisit CaptureCallState [v4]

ExE Boss duke at openjdk.org
Mon May 5 05:40:47 UTC 2025


On Mon, 5 May 2025 04:50:30 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:
> 
>   No env to test

Note that the use of the unsequenced `Map.of(…)` results in the return value of `CapturableState​.displayString(…)` to no longer be ordered by the mask bits.

To fix this, simply add a `private static List<CapturableState>`[^1] for storing the supported values:

[^1]: And/or introduce and use unmodifiable `SequencedMap.of(…)` and `SequencedSet.of(…)` factory methods.

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:         }

Suggestion:

    private static final List<CapturableState> SUPPORTED;

    static {
        final List<CapturableState> supported;

        if (OperatingSystem.isWindows()) {
            supported = List.of(
                    new CapturableState("GetLastError",    JAVA_INT, 1 << 0),
                    new CapturableState("WSAGetLastError", JAVA_INT, 1 << 1),
                    new CapturableState("errno",           JAVA_INT, 1 << 2)
            );
        } else {
            supported = List.of(new CapturableState("errno", JAVA_INT, 1 << 2));
        }

        SUPPORTED = supported;

src/java.base/share/classes/jdk/internal/foreign/abi/CapturableState.java line 96:

> 94:     public static String displayString(int mask) {
> 95:         var displayList = new ArrayList<>();
> 96:         for (var e : LOOKUP.values()) {

Suggestion:

        for (var e : SUPPORTED) {

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

PR Review: https://git.openjdk.org/jdk/pull/25025#pullrequestreview-2813828585
PR Review Comment: https://git.openjdk.org/jdk/pull/25025#discussion_r2072860691
PR Review Comment: https://git.openjdk.org/jdk/pull/25025#discussion_r2072860830


More information about the core-libs-dev mailing list