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

[Thomas Stuefe]

On Thu, 12 Jun 2025 14:01:14 GMT, Anton Artemov <duke at openjdk.org> wrote:

>> Hi,
>> 
>> in this PR the output value type for functions which return memory are changed, namely:
>> 
>> 
>> static julong available_memory(); --> static MemRes available_memory();
>> static julong used_memory(); --> static MemRes used_memory();
>> static julong free_memory(); --> static MemRes free_memory(); 
>> static jlong total_swap_space(); --> static MemRes total_swap_space();
>> static jlong free_swap_space(); --> static MemRes free_swap_space(); 
>> static julong physical_memory(); --> static MemRes physical_memory(); 
>> 
>> 
>> `MemRes` is a struct containing a pair of values, `size_t val` to carry the return value, `int err` to carry the error if any. Currently, in case of error the latter is set to -1.
>> 
>> The changes are done so that the other parts of the code have minimal impact. 
>> Tested in GHA and Tiers 1-4.
>
> Anton Artemov has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 17 commits:
> 
>  - 8357086: Fixed merge conflict.
>  - 8357086: Changed returm type to struct.
>  - 8357086: Return size_t from swap mem funcs, added checks.
>  - 8357086: Added missed casts.
>  - 8357086: Changed return type for total_swap_space and free_swap_space to ssize_t
>  - Merge remote-tracking branch 'origin/master' into JDK-8357086_size_t_memfuncs
>  - 8357086: Fixed spaces in formatting in gc-related code.
>  - 8357086: Fixed formatting.
>  - 8357086: Addressed reviewer's comments.
>  - 8357086: More work.
>  - ... and 7 more: https://git.openjdk.org/jdk/compare/e18277b4...dd9275ca

The return of a structure is an improvement over ssize_t. 

But I fear it does not go far enough in addressing @stefank 's problem, which is that people may find it too easy to ignore error returns. This PR shows just that by exchanging xx with xx.val and and mostly ignoring xx.err. I expect this will be a pattern too easy to copy.

An alternative would be this:


bool available_memory(size_t* out);


Follows hotspot conventions, which is to return `bool` as error flag, allows for a nicer flow at the call site, and forces the caller to think about the error. 


if (os:: available_memory(&s)) {
  // ok
} else {
  // need to think
}
...


If we define the function contract as "in case of error, leave the input size alone", the caller can pre-populate it with a reasonable default value:


size_t s = reasonable_default;
os:: available_memory(&s);
// use s for whatever
...


or maybe simplify the coding, e.g. when printing values:


size_t s = 0; // aka don't know
os::free_swap_space(&s);
st->print_cr("xxx: %zu", s);
...


-----

But a different question is which functions need this. In some cases we may be better off just ending the JVM with a fatal error right away in the function itself:
- if there is no way to handle the error meaningfully. Arguably os::physical_memory() is such a case: if that does not work, I doubt the JVM will live beyond initialization.
- if that function failing is a sign that something is very off. E.g. I think we can require that /proc/meminfo exists and is readable. If not, something with this Linux box is badly broken and an administrator should look at it. 

Returning errors makes sense in cases for cases that can actually happen at runtime:
- if the customer machine misses certain optional capabilities; e.g. RssAnon in /proc/pid/status on Linux, which depends on kernel version
- possibly when getting the information can stall or fail due to unknown runtime conditions. Not so sure about this. E.g. for EAGAIN, we could retry, but we never do that today.

---

I am torn. I know fixing the error returns is a much larger scope that this PR originally aimed for, and don't want to hold it off. I just feel that adding an error report channel that maybe is not needed (available_memory) is a step sideways, especially if we still mostly ignore the error. 

But if others want this PR to proceed, I'm fine with it.

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

PR Comment: https://git.openjdk.org/jdk/pull/25450#issuecomment-2967252113