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

[David Holmes]

On Tue, 5 Aug 2025 08:26:30 GMT, Anton Artemov <duke at openjdk.org> wrote:

>> 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.

The assert should go after the syscall that we don't expect to fail, but which we have to tolerate the possibility of failing. That is the place where we can also report why the call failed.

I'm not sure what you mean about changing the usage pattern.

>> 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.

Sorry I don't see a connection to previous comment about the assert. You only need a local if you need to refer to something more than once.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/25450#discussion_r2255794954
PR Review Comment: https://git.openjdk.org/jdk/pull/25450#discussion_r2255796528