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

[Anton Artemov]

On Tue, 5 Aug 2025 04:39:49 GMT, David Holmes <dholmes 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 26 commits:
>> 
>>  - 8357086: Addressed reviewer's comments
>>  - Merge remote-tracking branch 'origin/master' into JDK-8357086_size_t_memfuncs
>>  - 8357086: Fixed merge conflict
>>  - 8357086: Removed extra line
>>  - 8357086: Made physical_memory() return size_t, added dummy ATTRIBUTE_NODISCARD
>>  - 8357086: Small fixes
>>  - 8357086: Made physical_memory() return void
>>  - 8357086: Fixed behavior of total_swap_space on Linux
>>  - 8357086: Addressed reviewer's comments.
>>  - 8357086: Fixed void conversion.
>>  - ... and 16 more: https://git.openjdk.org/jdk/compare/57553ca1...a3898f43
>
> src/hotspot/os/linux/os_linux.cpp line 297:
> 
>> 295:   if (OSContainer::is_containerized() && OSContainer::memory_and_swap_limit_in_bytes() > 0) {
>> 296:     if (OSContainer::memory_limit_in_bytes() > 0) {
>> 297:       value = static_cast<size_t>(OSContainer::memory_and_swap_limit_in_bytes() - OSContainer::memory_limit_in_bytes());
> 
> Pre-existing but we should really only calls these functions once and store the result in a local.

Addressed.

> src/hotspot/os/linux/os_linux.cpp line 328:
> 
>> 326:   bool host_free_swap_ok = host_free_swap_f(host_free_swap);
>> 327:   if (!total_swap_space_ok || !host_free_swap_ok) {
>> 328:     assert(false, "sysinfo failed ? ");
> 
> Pre-existing: the assert should be pushed down to where the `sysinfo` calls are so you could see which one failed - not that they can actually fail if called correctly.

I think this would change the usage pattern again. Do we want a failing assert  in every memory function in case of failure? I thought the consensus is that a false values returned by the function is indicating that. In this particular case it will be easy to trace which method failed as their results are stored as local variables.

> src/hotspot/os/linux/os_linux.cpp line 330:
> 
>> 328:     assert(false, "sysinfo failed ? ");
>> 329:     return false;
>> 330:   }
> 
> Suggestion:
> 
>   size_t total_swap_space = 0;
>   size_t host_free_swap = 0;
>   if (!os::total_swap_space(total_swap_space) || !host_free_swap_f(host_free_swap)) {
>     assert(false, "sysinfo failed ? ");
>     return false;
>   }

I suggest to keep results as locals, see my previous comment.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/25450#discussion_r2253544122
PR Review Comment: https://git.openjdk.org/jdk/pull/25450#discussion_r2253543879
PR Review Comment: https://git.openjdk.org/jdk/pull/25450#discussion_r2253543435