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

[Anton Artemov]

On Thu, 12 Jun 2025 18:07:35 GMT, Kim Barrett <kbarrett 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 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
>
> 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.

Thanks, I did not think about it that way. I addressed the problem by sequential checking of errors.

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

Good suggestion, in most cases we don't need the 2nd one, I added a default value. Thanks.

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

I removed the default constructor, as decided to construct in the return statement with given argument(s).

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

PR Review Comment: https://git.openjdk.org/jdk/pull/25450#discussion_r2144698403
PR Review Comment: https://git.openjdk.org/jdk/pull/25450#discussion_r2144694805
PR Review Comment: https://git.openjdk.org/jdk/pull/25450#discussion_r2144696837