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

[Stefan Karlsson]

On Wed, 30 Jul 2025 15:22:39 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 incrementally with one additional commit since the last revision:
> 
>   8357086: Fixed void conversion.

>From my POV I think this starts to look OK except for a few nits.

I have a more lengthy comment below in Arguments::set_heap_size where I'm wondering if we really need to be returning an error in os::pysical_memory(). The question is mostly directed to Microsoft devs who might have insight into the memory API we use to query to get amount physical memory.

We could treat that question as a follow-up question and then maybe clean out the added error handling code (and places ignoring the error). Or we could try to resolve this before this PR is pushed so that we don't temporarily add all this extra error handling/ignoring code. What do you (and other reviewers) think?

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

> 68:   } else {
> 69:     log_info_p(gc, init)("Memory: NA");
> 70:   }

Suggestion:

  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 38:

> 36:   } else {
> 37:     log_info_p(gc, init)("Memory: NA");
> 38:   }

Restore removed blank line and renamed abbreviated double-word local. I'm a bit extra picky with the code in ZGC (and GC):
Suggestion:


  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/runtime/arguments.cpp line 1510:

> 1508: void Arguments::set_heap_size() {
> 1509:   julong phys_mem;
> 1510:   size_t physical_mem_val = 0;

Restore blankline:
Suggestion:

  size_t physical_mem_val = 0;

src/hotspot/share/runtime/arguments.cpp line 1523:

> 1521:   if (override_coop_limit) {
> 1522:     if (FLAG_IS_DEFAULT(MaxRAM)) {
> 1523:       (void)os::physical_memory(physical_mem_val);

This is one of the places where things seems to break down if we ever get a failure from os::physical_memory(). Before this patch I guess you would get random values and after this patch you will propagate 0 through this function. So, I guess it is not worse than before, but if we really can get a failure here then we do have an existing bug here.

With that said, when looking at this I'm wondering if there is any scenario where we can hit an error when calling the os::physical_memory? I understand that the OS APIs says that we can hit an error, but I wonder if that's only a theoretical issue? If it turns out that the APIs in effect never will fail then we can verify that they succeed during the JVM initialization and then remove the error code from os os::physical_memory() and let it only return a size_t instead. If we do this we can clean away a lot of the error handling code that is added in this PR.

The places that I see that could theoretically fail is:

1) On AIX, but there's an `assert(0` in there to show that this really doesn't happen.

2) On Windows if GlobalMemoryStatusEx(&ms) fails. The question is will this call fail for any reason on Windows? Maybe someone from Microsoft could help and tell if there ever is a reason why this code would fail:

  MEMORYSTATUSEX ms;
  ms.dwLength = sizeof(ms);
  GlobalMemoryStatusEx(&ms);

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

Changes requested by stefank (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/25450#pullrequestreview-3072202390
PR Review Comment: https://git.openjdk.org/jdk/pull/25450#discussion_r2243105949
PR Review Comment: https://git.openjdk.org/jdk/pull/25450#discussion_r2243109098
PR Review Comment: https://git.openjdk.org/jdk/pull/25450#discussion_r2243112602
PR Review Comment: https://git.openjdk.org/jdk/pull/25450#discussion_r2243184566