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

[Anton Artemov]

On Fri, 15 Aug 2025 10:51:12 GMT, Stefan Karlsson <stefank at openjdk.org> wrote:

>> Anton Artemov has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   8357086: Fixed whitespace error
>
> 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?

No, there is no reason, let's catch this asap.

> 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?

Addressed.

> 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?

Addressed.

> 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?

As per discussion above `GlobalMemoryStatusEx` seems to never fail. Let's be consistent and add the assert here as well.

> 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();

Addressed.

> 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();`.

Addressed.

> 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?

Okay, moved it up.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/25450#discussion_r2284508387
PR Review Comment: https://git.openjdk.org/jdk/pull/25450#discussion_r2284508133
PR Review Comment: https://git.openjdk.org/jdk/pull/25450#discussion_r2284508241
PR Review Comment: https://git.openjdk.org/jdk/pull/25450#discussion_r2284507883
PR Review Comment: https://git.openjdk.org/jdk/pull/25450#discussion_r2284507573
PR Review Comment: https://git.openjdk.org/jdk/pull/25450#discussion_r2284507410
PR Review Comment: https://git.openjdk.org/jdk/pull/25450#discussion_r2284507033