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

[Stefan Karlsson]

On Wed, 28 May 2025 11:19:10 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 size_t available_memory();
>> static julong used_memory(); --> static size_t used_memory();
>> static julong free_memory(); --> static size_t free_memory(); 
>> static jlong total_swap_space(); --> static ptrdiff_t total_swap_space();
>> static jlong free_swap_space(); --> static ptrdiff_t free_swap_space(); 
>> static julong physical_memory(); --> static size_t physical_memory(); 
>> 
>> 
>> The changes are done so that the other parts of the code have minimal impact. 
>> Tested in GHA and Tiers 1-4.
>> 
>> ---------
>> ### Progress
>> - [ ] Change must be properly reviewed (1 review required, with at least 1 [Reviewer](https://openjdk.org/bylaws#reviewer))
>> - [x] Change must not contain extraneous whitespace
>> - [x] Commit message must refer to an issue
>> 
>> ### Error
>>  ⚠️ The pull request body must not be empty.
>> 
>> 
>> 
>> ### Reviewing
>> <details><summary>Using <code>git</code></summary>
>> 
>> Checkout this PR locally: \
>> `$ git fetch https://git.openjdk.org/jdk.git pull/25450/head:pull/25450` \
>> `$ git checkout pull/25450`
>> 
>> Update a local copy of the PR: \
>> `$ git checkout pull/25450` \
>> `$ git pull https://git.openjdk.org/jdk.git pull/25450/head`
>> 
>> </details>
>> <details><summary>Using Skara CLI tools</summary>
>> 
>> Checkout this PR locally: \
>> `$ git pr checkout 25450`
>> 
>> View PR using the GUI difftool: \
>> `$ git pr show -t 25450`
>> 
>> </details>
>> <details><summary>Using diff file</summary>
>> 
>> Download this PR as a diff file: \
>> <a href="https://git.openjdk.org/jdk/pull/25450.diff">https://git.openjdk.org/jdk/pull/25450.diff</a>
>> 
>> </details>
>
> Anton Artemov has updated the pull request incrementally with one additional commit since the last revision:
> 
>   8357086: Fixed formatting.

Thanks for fixing the signed vs unsigned types, I'm much more confident with that.

I talked to Anton offline about the `ptrdiff_t`. That type has the correct signedness and number of bits on all our platforms, but to me that type carries a semantic meaning about what pointer diffs / indices. Because of that I found it inappropriate to use that type. I was expecting to see `ssize_t` being used, but that type is only guaranteed to hold [-1, SSIZE_MAX] and we have paths through the container code that returns -2. The man pages say "It is a signed integer type capable of storing values at least in the range [-1, SSIZE_MAX]", so there's an "at least" which might indicate that some systems allow -2? Maybe all our platform does that? Could that be tested with some kind of static asserts or unit test? Could others comment on what the correct type should be for these functions? Maybe @kimbarrett has a good idea about what type to use?

An alternative could be to let these functions return a compound value [size_t size, int error_code], or some adhoc optional type. I think that would be the safest way forward for these.

Looking forward to here from other reviewers.

Anton, I know that you intentionally left the code to be `"%zu" "k"` and I'm not going to argue strongly against that in the RT code, but I don't really want that in the ZGC code (or the GC code) so I added a couple of suggestions to change that.

src/hotspot/share/gc/shared/gcInitLogger.cpp line 66:

> 64: void GCInitLogger::print_memory() {
> 65:   size_t memory = os::physical_memory();
> 66:   log_info_p(gc, init)("Memory: %zu" "%s",

Suggestion:

  log_info_p(gc, init)("Memory: %zu%s",

src/hotspot/share/gc/z/zLargePages.cpp line 34:

> 32:   pd_initialize();
> 33: 
> 34:   log_info_p(gc, init)("Memory: %zu" "M", os::physical_memory() / M);

Suggestion:

  log_info_p(gc, init)("Memory: %zuM", os::physical_memory() / M);

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

PR Review: https://git.openjdk.org/jdk/pull/25450#pullrequestreview-2874802877
PR Review Comment: https://git.openjdk.org/jdk/pull/25450#discussion_r2111717945
PR Review Comment: https://git.openjdk.org/jdk/pull/25450#discussion_r2111718392