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

[Stefan Karlsson]

On Mon, 26 May 2025 12:56:26 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 size_t total_swap_space();
> static jlong free_swap_space(); --> static size_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>

Great to see removal of usages of the j* types where the C++ doesn't directly interface with the Java code.

I'm a little worried about some of the changes from signed to unsigned. Esp. when -1 is used to report an error case. Could you take an extra look at that and either fix it, add verification, and/or comments explaining why they are correct.

src/hotspot/os/bsd/os_bsd.cpp line 180:

> 178: }
> 179: 
> 180: size_t os::total_swap_space() {

Do you need to cast the `-1`in the changed functions in this file?

src/hotspot/os/linux/cgroupSubsystem_linux.cpp line 673:

> 671:   }
> 672:   size_t phys_mem = os::Linux::physical_memory();
> 673:   log_trace(os, container)("total physical memory: " "%zu", phys_mem);

Suggestion:

  log_trace(os, container)("total physical memory: %zu", phys_mem);

src/hotspot/os/linux/os_linux.cpp line 298:

> 296: }
> 297: 
> 298: static size_t host_free_swap() {

It is not obvious to me that callers of this are reacting properly when the return value goes from signed to unsigned. I'm thinking about code in os::free_swap_space below:

jlong host_free_swap_val = MIN2(os::total_swap_space(), host_free_swap());

How does MIN2 react to -1 vs 0xFFFFFFFFFFFFFFFF

src/hotspot/os/linux/os_linux.cpp line 349:

> 347: 
> 348:   phys_mem = Linux::physical_memory();
> 349:   log_trace(os)("total system memory: " "%zu", phys_mem);

Suggestion:

  log_trace(os)("total system memory: %zu", phys_mem);

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

Changes requested by stefank (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/25450#pullrequestreview-2874331377
PR Review Comment: https://git.openjdk.org/jdk/pull/25450#discussion_r2111415192
PR Review Comment: https://git.openjdk.org/jdk/pull/25450#discussion_r2111416853
PR Review Comment: https://git.openjdk.org/jdk/pull/25450#discussion_r2111425109
PR Review Comment: https://git.openjdk.org/jdk/pull/25450#discussion_r2111425748