On Wed, 19 Apr 2023 06:14:37 GMT, David Holmes <[email protected]> wrote:
>> Aleksey Shipilev has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Fix gtests
>
> src/hotspot/os/posix/os_posix.cpp line 1545:
>
>> 1543:
>> 1544: int PlatformEvent::park_nanos(jlong nanos) {
>> 1545: assert(0 <= nanos, "nanos are in range");
>
> `nanos` should never be zero else you call the untimed park.
OK, I see how is that guaranteed in the Windows case. In POSIX case, calling
`park()` is untimed wait, but `park(0)` is converted to absolute time that is
already passed, and so `pthread_cond_timedwait` would return immediately,
right? So `park(0)` is not equivalent to just `park()`? Still, the strongest
behavior from Windows case takes precedence here. Changed the assert.
> src/hotspot/os/posix/park_posix.hpp line 57:
>
>> 55: void park();
>> 56: int park_millis(jlong millis);
>> 57: int park_nanos(jlong nanos);
>
> Still not sure we need this API split but if we keep `park(jlong millis)` and
> just add `park_nanos(jlong nanos)` then you can avoid touching so many places
> in the code.
I thought the exposure to `park` -> `park_millis` renames would be smaller, but
apparently there is a considerable number of uses. I left `park(millis)` (old)
and added `park_nanos(nanos)` (new), and reverted `park_millis` changes.
> src/hotspot/share/runtime/javaThread.hpp line 1145:
>
>> 1143: public:
>> 1144: bool sleep_millis(jlong millis);
>> 1145: bool sleep_nanos(jlong nanos);
>
> I prefer just one sleep that takes nanos.
If we do only `sleep(jlong nanos)`, then there is an accident waiting to
happen, when some unfixed code would call `sleep` with `millis` argument, not
knowing it is now `nanos`. That was the reason why I made the names explicit.
`sleep_millis` also does the conversion to nanos that does not overflow. But,
like with `park` above, I think there is an argument to keep `sleep(millis)`
and add `sleep_nanos(nanos)`, to keep code changes at minimum.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/13225#discussion_r1171103473
PR Review Comment: https://git.openjdk.org/jdk/pull/13225#discussion_r1171103664
PR Review Comment: https://git.openjdk.org/jdk/pull/13225#discussion_r1171103560
