On Mon, 5 May 2025 15:52:38 GMT, Chen Liang <li...@openjdk.org> wrote:
>> 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.
>
> Enum.valueOf -> Class.enumConstantDirectory -> Class.getEnumConstantsShared
> -> Method.invoke ->
> MethodHandleAccessorFactory.makeSpecializedTarget(isStatic = true) ->
> MethodHandles.dropArguments -> LambdaForm.editor -> bytecode generation and
> loading because this currently cannot be pregenerated by CDS archive.
>
> If this class is broken, this would probably already surface at build time
> because this is used by jlink; otherwise it would have surfaced in Process
> tests. I don't think ensuring EIIE vs IAE is worth a test here.
Thank you Chen for updating the issue type of bug.
> If this class is broken, this would probably already surface at build time
> because this is used by jlink; otherwise it would have surfaced in Process
> tests. I don't think ensuring EIIE vs IAE is worth a test here.
The current bug fix I think should just address the incorrect result from
`captureCallState()`.
If the change to OperatingSystem class has practical improvements to the
startup performance, then I think it's worth proposing. I suggest we do it in a
separate and independent PR and the discussion and review for that change would
have to take into account the existing comment in that class and see if it is
no longer necessary.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/25025#discussion_r2074814448
