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

Tue Jul 29 11:59:04 UTC 2025

On Fri, 27 Jun 2025 07:30:26 GMT, Anton Artemov <duke at openjdk.org> wrote:

>> Hi,
>> 
>> in this PR the output value types for functions which return memory are changed, namely:
>> 
>> 
>> static julong available_memory(); --> static bool available_memory(size_t& value);
>> static julong used_memory(); --> static bool used_memory(size_t& value);
>> static julong free_memory(); --> static bool free_memory(size_t& value); 
>> static jlong total_swap_space(); --> static bool total_swap_space(size_t& value);
>> static jlong free_swap_space(); --> static bool free_swap_space(size_t& value); 
>> static julong physical_memory(); --> static bool physical_memory(size_t& value); 
>> 
>> 
>> The return boolean value indicates success, whereas the actual value is assigned to the input argument. The following recommended usage pattern is introduced: where applicable, and unsuccessful call is logged. 
>> 
>> Later, the return value can be attributed with `[[nodiscard]]` to enforce the pattern.
>> 
>> Tested in GHA and Tiers 1-5.
>
> Anton Artemov has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 12 commits:
> 
>  - 8357086: Fxied return value
>  - 8357086: Fixed whitespaces
>  - 8357086: Introduced usage pattern
>  - 8357086: Fixed typo
>  - 8357086: Refactored physical_memory in different OS
>  - 8357086: Small fixes 2
>  - 8357086: Small fixes 1.
>  - 8357086: Refactored physical_memory()
>  - 8357086: Refactored free_swap_space()
>  - 8357086: Refactored total_swap_space()
>  - ... and 2 more: https://git.openjdk.org/jdk/compare/75ce44aa...e4698333

I went over this and have some more comments. The two main concerns I have are:
1) I really would like to see all the UL "X failed" log lines removed from this patch.
2) Callers that discard the return value should be clearly marked that they do so. (Alternatively, provide APIs that clearly returns 0 on failure).

src/hotspot/os/aix/os_aix.cpp line 296:

> 294:   size_t phys_mem = Aix::physical_memory();
> 295:   if (phys_mem == std::numeric_limits<size_t>::max())
> 296:   {

Suggestion:

  if (phys_mem == std::numeric_limits<size_t>::max()) {

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

> 158:     available = (vmstat.free_count + vmstat.inactive_count + vmstat.purgeable_count) * os::vm_page_size();
> 159:   } else {
> 160:     return false;

This looks like a change in behavior, but I guess it is a situation we shouldn't encounter or we would have hit the assert above.

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

> 288:   if (sysctl(mib, 2, &mem_val, &len, nullptr, 0) != -1) {
> 289:     assert(len == sizeof(mem_val), "unexpected data size");
> 290:     _physical_memory = static_cast<size_t>(mem_val);

The `__OpenBSD__` section below still uses `julong`.

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

> 1475:   if (!os::physical_memory(phys_mem)) {
> 1476:     log_debug(os, thread)("os::physical_memory() failed");
> 1477:   }

It's not clear to me that we want to have a side-effect of logging to UL here. Wouldn't it make more sense to log that physical memory info is not available to the provided `st` stream?

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

> 1480:   size_t avail_mem = 0;
> 1481:   if (!os::available_memory(avail_mem))
> 1482:   {

Suggestion:

  if (!os::available_memory(avail_mem)) {

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

> 295:   if (OSContainer::is_containerized()) {
> 296:     if (OSContainer::memory_limit_in_bytes() > 0) {
> 297:       value = static_cast<size_t>(OSContainer::memory_and_swap_limit_in_bytes() - OSContainer::memory_limit_in_bytes());

I might have wondered about this before, but can't `memory_and_swap_limit_in_bytes` return `-1` even when `memory_limit_in_bytes > 0` since -1 is treated as an "unlimited limit" here:

jlong CgroupV1MemoryController::read_mem_swap(julong host_total_memsw) {
  julong memswlimit;
  CONTAINER_READ_NUMBER_CHECKED(reader(), "/memory.memsw.limit_in_bytes", "Memory and Swap Limit", memswlimit);
  if (memswlimit >= host_total_memsw) {
    log_trace(os, container)("Memory and Swap Limit is: Unlimited");
    return (jlong)-1;
  }

Maybe this will be handled in a cleanup to the container subsystem?

src/hotspot/os/windows/os_windows.cpp line 900:

> 898:   size_t phys_mem = win32::physical_memory();
> 899:   if (phys_mem == std::numeric_limits<size_t>::max())
> 900:   {

Suggestion:

  if (phys_mem == std::numeric_limits<size_t>::max()) {

src/hotspot/share/compiler/compileBroker.cpp line 1062:

> 1060:   // Now, we do the more expensive operations.
> 1061:   size_t free_memory = 0;
> 1062:   os::free_memory(free_memory);

I wonder if this should be changed to something like this:
Suggestion:

  // Return value ignored - defaulting to 0 on failure.
  (void)os::free_memory(free_memory);

to clearly indicate the intent to skip the error check?

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

> 68:     log_debug_p(gc, init)("os::physical_memory() failed");
> 69:   }
> 70:   julong memory = static_cast<julong>(phys_mem);

I would prefer to not have the extra logging in the GC code + some extra restructuring:

  size_t memory = 0;
  if (os::physical_memory(memory)) {
    log_info_p(gc, init)("Memory: %zu%s",
                         byte_size_in_proper_unit(memory), proper_unit_for_byte_size(memory));
  } else {
    log_info_p(gc, init)("Memory: NA");
  }

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

> 35:     log_debug_p(gc, init)("os::physical_memory() failed");
> 36:   }
> 37:   log_info_p(gc, init)("Memory: %zuM", phys_mem / M);

I have the same comment about this change as I have for `GCInitLogger::print_memory`.

src/hotspot/share/jfr/jni/jfrJniMethod.cpp line 406:

> 404: 
> 405: JVM_ENTRY_NO_ENV(jlong, jfr_host_total_memory(JNIEnv* env, jclass jvm))
> 406:   size_t phys_mem = 0;

This can be moved below where it is used. I would probably also be good to explicitly show that the return value is skipped.

With that said, now I've seen this pattern a number of times and think that it might be cleaner to have a function named `os::physical_memory_or_0()` (just like we have a bunch of "_or_null" functions) that returns 0 on failure. The code could then be:

#ifdef LINUX
  // We want the host memory, not the container limit.
  // os::physical_memory() would return the container limit.
  return os::Linux::physical_memory();
#else
  return os::physical_memory_or_0();
#endif

We can think about that proposal as a follow-up RFE.

src/hotspot/share/prims/whitebox.cpp line 2515:

> 2513:   LINUX_ONLY(return os::Linux::physical_memory();)
> 2514:   os::physical_memory(phys_mem);
> 2515:   return static_cast<jlong>(phys_mem);

This looks like the JFR code that doesn't want to report the container limits. We should probably have an os:: function that returns this value that both these functions wants. Maybe `os::host_physical_memory`. Something for a future RFE.

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

Changes requested by stefank (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/25450#pullrequestreview-3066796623
PR Review Comment: https://git.openjdk.org/jdk/pull/25450#discussion_r2239340170
PR Review Comment: https://git.openjdk.org/jdk/pull/25450#discussion_r2239354684
PR Review Comment: https://git.openjdk.org/jdk/pull/25450#discussion_r2239388371
PR Review Comment: https://git.openjdk.org/jdk/pull/25450#discussion_r2239394003
PR Review Comment: https://git.openjdk.org/jdk/pull/25450#discussion_r2239389671
PR Review Comment: https://git.openjdk.org/jdk/pull/25450#discussion_r2239429254
PR Review Comment: https://git.openjdk.org/jdk/pull/25450#discussion_r2239466558
PR Review Comment: https://git.openjdk.org/jdk/pull/25450#discussion_r2239489983
PR Review Comment: https://git.openjdk.org/jdk/pull/25450#discussion_r2239528661
PR Review Comment: https://git.openjdk.org/jdk/pull/25450#discussion_r2239540372
PR Review Comment: https://git.openjdk.org/jdk/pull/25450#discussion_r2239550486
PR Review Comment: https://git.openjdk.org/jdk/pull/25450#discussion_r2239557604