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

[Stefan Karlsson]

On Wed, 6 Aug 2025 09:48:28 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 size_t physical_memory(size_t& value); 
>> 
>> 
>> The return boolean value, where available, indicates success, whereas the actual value is assigned to the input argument. The following recommended usage pattern is introduced: where applicable, an unsuccessful call is logged. 
>> 
>> `ATTRIBUTE_NODISCARD` macro is added as a placeholder for `[[nodiscard]]`, which will be available with C++17.
>> 
>> 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 whitespace error

I looked over the patch and listed a few more small requests. Other than that I think this PR looks fine from my POV. There are a few follow-up cleanup and fixes we should do, but I don't think those need to be handled in this PR.

What do other Reviewers think? Is this getting ready to be integrated soon?

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

> 868:   } else {
> 869:     DWORD errcode  = ::GetLastError();
> 870:     log_debug(os)("available_memory() failed to GlobalMemoryStatusEx: GetLastError->%lu.", errcode);

I think this should be an assert. Or is there a reason why that would be bad?

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

> 882:   } else {
> 883:     DWORD errcode  = ::GetLastError();
> 884:     log_debug(os)("total_swap_space() failed to GlobalMemoryStatusEx: GetLastError->%lu.", errcode);

assert?

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

> 896:   } else {
> 897:     DWORD errcode  = ::GetLastError();
> 898:     log_debug(os)("free_swap_space() failed to GlobalMemoryStatusEx: GetLastError->%lu.", errcode);

assert?

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

> 4166:   // also returns dwAvailPhys (free physical memory bytes), dwTotalVirtual, dwAvailVirtual,
> 4167:   // dwMemoryLoad (% of memory in use)
> 4168:   BOOL res = GlobalMemoryStatusEx(&ms);

Unused local variable? Should there be an assert here?

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

> 32:   pd_initialize();
> 33: 
> 34:   size_t memory = os::physical_memory();

ZGC coding style for locals:
Suggestion:

  const size_t memory = os::physical_memory();

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

> 415:   return os::Linux::physical_memory();
> 416: #else
> 417:   return static_cast<jlong>(os::physical_memory());

The two paths are inconsistent. This casts the return but the lines above doesn't cast the return value of `os::Linux::physical_memory();`.

src/hotspot/share/utilities/globalDefinitions.hpp line 1366:

> 1364: 
> 1365: // Dummy placeholder for use of [[nodiscard]]
> 1366: #define ATTRIBUTE_NODISCARD

Should this be placed near the other listed ATTRIBUTES in this file?

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

Changes requested by stefank (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/25450#pullrequestreview-3123593306
PR Review Comment: https://git.openjdk.org/jdk/pull/25450#discussion_r2278787309
PR Review Comment: https://git.openjdk.org/jdk/pull/25450#discussion_r2278787627
PR Review Comment: https://git.openjdk.org/jdk/pull/25450#discussion_r2278787491
PR Review Comment: https://git.openjdk.org/jdk/pull/25450#discussion_r2278788547
PR Review Comment: https://git.openjdk.org/jdk/pull/25450#discussion_r2278791660
PR Review Comment: https://git.openjdk.org/jdk/pull/25450#discussion_r2278794078
PR Review Comment: https://git.openjdk.org/jdk/pull/25450#discussion_r2278799119