RFR: 8357086: os::xxx functions returning memory size should return size_t [v13]

[Anton Artemov]

On Tue, 29 Jul 2025 10:39:07 GMT, Stefan Karlsson <stefank at openjdk.org> wrote:

>> Anton Artemov has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 12 commits:
>> 
>>  - 8357086: Fxied return value
>>  - 8357086: Fixed whitespaces
>>  - 8357086: Introduced usage pattern
>>  - 8357086: Fixed typo
>>  - 8357086: Refactored physical_memory in different OS
>>  - 8357086: Small fixes 2
>>  - 8357086: Small fixes 1.
>>  - 8357086: Refactored physical_memory()
>>  - 8357086: Refactored free_swap_space()
>>  - 8357086: Refactored total_swap_space()
>>  - ... and 2 more: https://git.openjdk.org/jdk/compare/75ce44aa...e4698333
>
> src/hotspot/os/bsd/os_bsd.cpp line 160:
> 
>> 158:     available = (vmstat.free_count + vmstat.inactive_count + vmstat.purgeable_count) * os::vm_page_size();
>> 159:   } else {
>> 160:     return false;
> 
> This looks like a change in behavior, but I guess it is a situation we shouldn't encounter or we would have hit the assert above.

I made it to be consistent with behavior of this method implementation on other platforms. Yes, the assert just about it should be hit before that.

> src/hotspot/share/jfr/jni/jfrJniMethod.cpp line 406:
> 
>> 404: 
>> 405: JVM_ENTRY_NO_ENV(jlong, jfr_host_total_memory(JNIEnv* env, jclass jvm))
>> 406:   size_t phys_mem = 0;
> 
> This can be moved below where it is used. I would probably also be good to explicitly show that the return value is skipped.
> 
> With that said, now I've seen this pattern a number of times and think that it might be cleaner to have a function named `os::physical_memory_or_0()` (just like we have a bunch of "_or_null" functions) that returns 0 on failure. The code could then be:
> 
> #ifdef LINUX
>   // We want the host memory, not the container limit.
>   // os::physical_memory() would return the container limit.
>   return os::Linux::physical_memory();
> #else
>   return os::physical_memory_or_0();
> #endif
> 
> We can think about that proposal as a follow-up RFE.

I do not agree that it is clearer to have a function named   `os::physical_memory_or_0()`, because 0 does not necessarily mean a failure. It may well be a valid value for something on a system level. Not even touching various posix stuff returning 0 in case of success.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/25450#discussion_r2242800170
PR Review Comment: https://git.openjdk.org/jdk/pull/25450#discussion_r2242797831