RFR: 8356126: Revisit CaptureCallState [v4]
Jaikiran Pai
jpai at openjdk.org
Mon May 5 08:18:46 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
src/java.base/share/classes/jdk/internal/util/OperatingSystem.java line 139:
> 137: */
> 138: private static OperatingSystem initOS() {
> 139: // Called lazily, valueOf has overhead
Hello Chen, in context of this bug fix, what kind of overhead does `valueOf()` have?
The (pre-existing) comment on this `initOS()` method sets an expectation that it will be called from the static initializer of this `OperatingSystem` class and thus it expects an `ExceptionInInitializerError` to be thrown by the static initiliazer, if the operating system name isn't recognized. Any access to `OperatingSystem` class would then have resulted in a `NoClassDefFoundError`. With this proposed change, the callers of `OperatingSystem.current()` would now start seeing an `IllegalArgumentException` if for any reason the operating system name isn't recognized.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/25025#discussion_r2073026614
More information about the core-libs-dev
mailing list