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

[Kim Barrett]

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

Tons of MemErr's without examining the error part at all.  I guess that's a
consequence of the existing code just not bothering to check, which I guess
needs to be fixed but not in this PR.

src/hotspot/share/runtime/os.cpp line 2217:

> 2215: #endif
> 2216:   res.val = os::physical_memory().val - os::available_memory().val;
> 2217:   res.err = MIN2(os::physical_memory().err, os::available_memory().err);

Repeated calls shouldn't be assumed to return the same error status.
And min seems like the wrong way to combine errors.  I think this should get the result from
`os::physical_memory` and inspect it for an error.  If not an error then do the same for
`os::available_memory`.  If not an error then compute the result accordingly.

src/hotspot/share/runtime/os.hpp line 151:

> 149: struct MemRes {
> 150:   size_t val;
> 151:   int err;

I'm not a big fan of abbreviations like this. My preference would be to at least spell out "value".
Don't know what others might think.

src/hotspot/share/runtime/os.hpp line 152:

> 150:   size_t val;
> 151:   int err;
> 152:   MemRes(size_t v, int e) : val(v), err(e) {}

It might be that having the second argument be optional and default to 0 is nicer / more convenient.

src/hotspot/share/runtime/os.hpp line 153:

> 151:   int err;
> 152:   MemRes(size_t v, int e) : val(v), err(e) {}
> 153:   MemRes(): val(0), err(0) {}

Do we really need initialization in the default ctor?  And if so, is default initialization to not-an-error
what we want?

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

Changes requested by kbarrett (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/25450#pullrequestreview-2922179889
PR Review Comment: https://git.openjdk.org/jdk/pull/25450#discussion_r2143353799
PR Review Comment: https://git.openjdk.org/jdk/pull/25450#discussion_r2143364103
PR Review Comment: https://git.openjdk.org/jdk/pull/25450#discussion_r2143362397
PR Review Comment: https://git.openjdk.org/jdk/pull/25450#discussion_r2143360927