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

[Anton Artemov]

On Tue, 5 Aug 2025 04:51:10 GMT, David Holmes <dholmes at openjdk.org> wrote:

>> Anton Artemov has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 26 commits:
>> 
>>  - 8357086: Addressed reviewer's comments
>>  - Merge remote-tracking branch 'origin/master' into JDK-8357086_size_t_memfuncs
>>  - 8357086: Fixed merge conflict
>>  - 8357086: Removed extra line
>>  - 8357086: Made physical_memory() return size_t, added dummy ATTRIBUTE_NODISCARD
>>  - 8357086: Small fixes
>>  - 8357086: Made physical_memory() return void
>>  - 8357086: Fixed behavior of total_swap_space on Linux
>>  - 8357086: Addressed reviewer's comments.
>>  - 8357086: Fixed void conversion.
>>  - ... and 16 more: https://git.openjdk.org/jdk/compare/57553ca1...a3898f43
>
> src/hotspot/os/windows/os_windows.cpp line 870:
> 
>> 868:   } else {
>> 869:     return false;
>> 870:   }
> 
> Suggestion:
> 
>   BOOL res = GlobalMemoryStatusEx(&ms);
>   if (res == TRUE) {
>     value = static_cast<size_t>(ms.ullAvailPhys);
>   }
>   return res == TRUE;
>   }
> 
> Unfortunately `BOOL` is an implicit boolean.
> 
> This applies to all the Windows functions.
> 
> You could also assert that `res == TRUE` and report `GetlastError` if not.

Addressed, though it looks like GlobalMemoryStatusEx never fails.

> src/hotspot/share/jfr/jni/jfrJniMethod.cpp line 418:
> 
>> 416: #else
>> 417:   size_t phys_mem = os::physical_memory();
>> 418:   return static_cast<jlong>(phys_mem);
> 
> Suggestion:
> 
>   return static_cast<jlong>(os::physical_memory());

Addressed.

> src/hotspot/share/jfr/periodic/jfrPeriodic.cpp line 532:
> 
>> 530: TRACE_REQUEST_FUNC(PhysicalMemory) {
>> 531:   size_t phys_mem = os::physical_memory();
>> 532:   u8 totalPhysicalMemory = static_cast<u8>(phys_mem);
> 
> Suggestion:
> 
>   u8 totalPhysicalMemory = static_cast<u8>(os::physical_memory());

Addressed.

> src/hotspot/share/jfr/periodic/jfrPeriodic.cpp line 536:
> 
>> 534:   event.set_totalSize(totalPhysicalMemory);
>> 535:   size_t avail_mem = 0;
>> 536:   (void)os::available_memory(avail_mem);
> 
> Suggestion:
> 
>   size_t avail_mem = 0;
>   // Return value ignored - defaulting to 0 on failure.
>   (void)os::available_memory(avail_mem);

Addressed.

> src/hotspot/share/jfr/periodic/jfrPeriodic.cpp line 547:
> 
>> 545:   event.set_totalSize(static_cast<s8>(total_swap_space));
>> 546:   size_t free_swap_space = 0;
>> 547:   (void)os::free_swap_space(free_swap_space);
> 
> Suggestion:
> 
>   size_t total_swap_space = 0;
>   // Return value ignored - defaulting to 0 on failure.
>   (void)os::total_swap_space(total_swap_space);
>   event.set_totalSize(static_cast<s8>(total_swap_space));
>   size_t free_swap_space = 0;
>   // Return value ignored - defaulting to 0 on failure.
>   (void)os::free_swap_space(free_swap_space);

Addressed.

> src/hotspot/share/prims/jvmtiRedefineClasses.cpp line 1362:
> 
>> 1360:     InstanceKlass* the_class = get_ik(_class_defs[i].klass);
>> 1361:     size_t avail_mem = 0;
>> 1362:     (void)os::available_memory(avail_mem);
> 
> Suggestion:
> 
>     size_t avail_mem = 0;
>     // Return value ignored - defaulting to 0 on failure.
>     (void)os::available_memory(avail_mem);

Addressed.

> src/hotspot/share/prims/jvmtiRedefineClasses.cpp line 1529:
> 
>> 1527:       }
>> 1528:     }
>> 1529:     (void)os::available_memory(avail_mem);
> 
> Suggestion:
> 
>     avail_mem = 0;
>     // Return value ignored - defaulting to 0 on failure.
>     (void)os::available_memory(avail_mem);

Addressed.

> src/hotspot/share/prims/jvmtiRedefineClasses.cpp line 4440:
> 
>> 4438:     // direct and indirect subclasses of the_class
>> 4439:     size_t avail_mem = 0;
>> 4440:     (void)os::available_memory(avail_mem);
> 
> Suggestion:
> 
>     size_t avail_mem = 0;
>     // Return value ignored - defaulting to 0 on failure.    
>     (void)os::available_memory(avail_mem);

Addressed.

> src/hotspot/share/prims/whitebox.cpp line 2516:
> 
>> 2514:   LINUX_ONLY(return static_cast<jlong>(os::Linux::physical_memory());)
>> 2515:   size_t phys_mem = os::physical_memory();
>> 2516:   return static_cast<jlong>(phys_mem);
> 
> Suggestion:
> 
>   return static_cast<jlong>(LINUX_ONLY(os::Linux::physical_memory()) NOT_LINUX(os::physical_memory()));
> 
> Though this seems like a bad code "smell" to me - there is something wrong with our notion of "physical memory" if we have to make this distinction.

Addressed.

> src/hotspot/share/services/heapDumper.cpp line 2616:
> 
>> 2614:     // Lets ensure we have at least 20MB per thread.
>> 2615:     size_t free_memory = 0;
>> 2616:     (void)os::free_memory(free_memory);
> 
> Suggestion:
> 
>     size_t free_memory = 0;
>     // Return value ignored - defaulting to 0 on failure.
>     (void)os::free_memory(free_memory);

Addressed.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/25450#discussion_r2253531025
PR Review Comment: https://git.openjdk.org/jdk/pull/25450#discussion_r2253530680
PR Review Comment: https://git.openjdk.org/jdk/pull/25450#discussion_r2253530386
PR Review Comment: https://git.openjdk.org/jdk/pull/25450#discussion_r2253529920
PR Review Comment: https://git.openjdk.org/jdk/pull/25450#discussion_r2253529670
PR Review Comment: https://git.openjdk.org/jdk/pull/25450#discussion_r2253529181
PR Review Comment: https://git.openjdk.org/jdk/pull/25450#discussion_r2253528912
PR Review Comment: https://git.openjdk.org/jdk/pull/25450#discussion_r2253528737
PR Review Comment: https://git.openjdk.org/jdk/pull/25450#discussion_r2253527398
PR Review Comment: https://git.openjdk.org/jdk/pull/25450#discussion_r2253527011