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

Tue Aug 5 05:19:17 UTC 2025

On Mon, 4 Aug 2025 08:43:48 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 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

Overall I'm okay with the way the API has turned out here. Naturally it would be better if no errors were possible, but I think containers prevent that from being the case.

I have a lot of stylistic comments, many pre-existing, and some notes for future cleanups. But happy to hit approve as a vote of confidence.

Thanks

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

> 135: 
> 136: bool os::available_memory(size_t& value) {
> 137:   return Bsd::available_memory(value);

Just an aside for a future cleanup, but there is really no reason to have the indirection through e.g. `Bsd::available_memory`.

src/hotspot/os/linux/cgroupSubsystem_linux.cpp line 675:

> 673:   julong phys_mem = static_cast<julong>(os::Linux::physical_memory());
> 674:   log_trace(os, container)("total physical memory: " JULONG_FORMAT, phys_mem);
> 675:   jlong mem_limit = contrl->controller()->read_memory_limit_in_bytes(phys_mem);

Another aside for a future cleanup: the container methods should be taking size_t not julong.

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

> 295:   if (OSContainer::is_containerized() && OSContainer::memory_and_swap_limit_in_bytes() > 0) {
> 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());

Pre-existing but we should really only calls these functions once and store the result in a local.

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

> 326:   bool host_free_swap_ok = host_free_swap_f(host_free_swap);
> 327:   if (!total_swap_space_ok || !host_free_swap_ok) {
> 328:     assert(false, "sysinfo failed ? ");

Pre-existing: the assert should be pushed down to where the `sysinfo` calls are so you could see which one failed - not that they can actually fail if called correctly.

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

> 328:     assert(false, "sysinfo failed ? ");
> 329:     return false;
> 330:   }

Suggestion:

  size_t total_swap_space = 0;
  size_t host_free_swap = 0;
  if (!os::total_swap_space(total_swap_space) || !host_free_swap_f(host_free_swap)) {
    assert(false, "sysinfo failed ? ");
    return false;
  }

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.

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

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

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

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

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

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

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

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.

src/hotspot/share/runtime/os.hpp line 343:

> 341: 
> 342:   static size_t physical_memory();
> 343:   static bool has_allocatable_memory_limit(size_t* limit);

Future cleanup: for consistency should this now also take a reference?

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

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

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

Stay-tuned we may actually be able to use `[[nodiscard]]` for real.

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

Marked as reviewed by dholmes (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/25450#pullrequestreview-3086467651
PR Review Comment: https://git.openjdk.org/jdk/pull/25450#discussion_r2253073149
PR Review Comment: https://git.openjdk.org/jdk/pull/25450#discussion_r2253077906
PR Review Comment: https://git.openjdk.org/jdk/pull/25450#discussion_r2253087855
PR Review Comment: https://git.openjdk.org/jdk/pull/25450#discussion_r2253092957
PR Review Comment: https://git.openjdk.org/jdk/pull/25450#discussion_r2253089924
PR Review Comment: https://git.openjdk.org/jdk/pull/25450#discussion_r2253099723
PR Review Comment: https://git.openjdk.org/jdk/pull/25450#discussion_r2253104891
PR Review Comment: https://git.openjdk.org/jdk/pull/25450#discussion_r2253106908
PR Review Comment: https://git.openjdk.org/jdk/pull/25450#discussion_r2253108762
PR Review Comment: https://git.openjdk.org/jdk/pull/25450#discussion_r2253109375
PR Review Comment: https://git.openjdk.org/jdk/pull/25450#discussion_r2253109727
PR Review Comment: https://git.openjdk.org/jdk/pull/25450#discussion_r2253112611
PR Review Comment: https://git.openjdk.org/jdk/pull/25450#discussion_r2253113166
PR Review Comment: https://git.openjdk.org/jdk/pull/25450#discussion_r2253115953
PR Review Comment: https://git.openjdk.org/jdk/pull/25450#discussion_r2253125260
PR Review Comment: https://git.openjdk.org/jdk/pull/25450#discussion_r2253126156
PR Review Comment: https://git.openjdk.org/jdk/pull/25450#discussion_r2253127032