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

Wed Jul 30 14:57:04 UTC 2025

On Wed, 30 Jul 2025 14:28:16 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: Addressed reviewer's comments

* I added some comments to the old resolved comments (Mentioning because they can be easy to miss).
* Some double-logging
* Small nits with the curly brackets
* There's still a bunch of logging that I don't think should be included.

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

> 69:     log_info_p(gc, init)("Memory: NA");
> 70:   }
> 71:   julong memory = static_cast<julong>(phys_mem);

Something went wrong here. The old logging is still left. Could you get rid of size_t ?`phys_mem` and only have a `size_t memory` variable?

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

> 35:     log_info_p(gc, init)("Memory: %zu%s", byte_size_in_proper_unit(phys_mem), proper_unit_for_byte_size(phys_mem));
> 36:   }
> 37:   else {

Suggestion:

  } else {

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

> 38:     log_info_p(gc, init)("Memory: NA");
> 39:   }
> 40:   log_info_p(gc, init)("Memory: %zuM", phys_mem / M);

There's double-logging here now.

src/hotspot/share/jfr/periodic/jfrPeriodic.cpp line 534:

> 532:   if (!os::physical_memory(phys_mem)) {
> 533:     log_debug(jfr, system)("os::physical_memory() failed");
> 534:   }

Suggestion:

  // Ignore return value
  (void)os::physical_memory(phys_mem);

src/hotspot/share/jfr/periodic/jfrPeriodic.cpp line 541:

> 539:   if (!os::available_memory(avail_mem)) {
> 540:     log_debug(jfr, system)("os::available_memory() failed");
> 541:   }

Suggestion:

  // Ignore return value
  (void)os::available_memory(avail_mem);

src/hotspot/share/jfr/periodic/jfrPeriodic.cpp line 551:

> 549:   if (!os::total_swap_space(total_swap_space)) {
> 550:     log_debug(jfr, system)("os::total_swap_space() failed");
> 551:   }

Suggestion:

  // Ignore return value
  (void)os::total_swap_space(total_swap_space);

src/hotspot/share/jfr/periodic/jfrPeriodic.cpp line 556:

> 554:   if (!os::free_swap_space(free_swap_space)) {
> 555:     log_debug(jfr, system)("os::free_swap_space() failed");
> 556:   }

Suggestion:

  // Ignore return value
  (void)os::free_swap_space(free_swap_space);

src/hotspot/share/prims/jvmtiRedefineClasses.cpp line 1364:

> 1362:     if (!os::available_memory(avail_mem)) {
> 1363:       log_debug(redefine, class, load)("os::available_memory() failed");
> 1364:     }

Suggestion:

    // Ignore return value
    (void)os::available_memory(avail_mem);

src/hotspot/share/prims/jvmtiRedefineClasses.cpp line 4446:

> 4444:     if (!os::available_memory(avail_mem)) {
> 4445:       log_debug(redefine, class, load)("os::available_memory() failed");
> 4446:     }

Suggestion:

    // Ignore return value
    (void)os::available_memory(avail_mem);

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

> 1523:       if (!os::physical_memory(physical_mem_val)) {
> 1524:         log_debug(os)("os::physical_memory() failed");
> 1525:       }

Suggestion:

      // Ignore return value
      (void)os::physical_memory(physical_mem_val);

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

> 1533:     if (!os::physical_memory(physical_mem_val)) {
> 1534:       log_debug(os)("os::physical_memory() failed");
> 1535:     }

Suggestion:

    // Ignore return value
    (void)os::physical_memory(physical_mem_val);

src/hotspot/share/runtime/os.cpp line 1189:

> 1187:   if (!physical_memory(phys_mem)) {
> 1188:     log_debug(os)("os::physical_memory() failed");
> 1189:   }

Suggestion:

  // Ignore return value
  (void)physical_memory(phys_mem);

src/hotspot/share/runtime/os.cpp line 1950:

> 1948:   if (!os::physical_memory(phys_mem)) {
> 1949:     log_debug(os)("os::physical_memory() failed");
> 1950:   }

Suggestion:

  // Ignore return value
  (void)os::physical_memory(phys_mem);

src/hotspot/share/services/heapDumper.cpp line 2618:

> 2616:     if (!os::free_memory(free_memory)) {
> 2617:       log_debug(heapdump)("os::free_memory() failed");
> 2618:     }

Suggestion:

    (void)os::free_memory(free_memory);

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

Changes requested by stefank (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/25450#pullrequestreview-3071953120
PR Review Comment: https://git.openjdk.org/jdk/pull/25450#discussion_r2242945249
PR Review Comment: https://git.openjdk.org/jdk/pull/25450#discussion_r2242946849
PR Review Comment: https://git.openjdk.org/jdk/pull/25450#discussion_r2242949676
PR Review Comment: https://git.openjdk.org/jdk/pull/25450#discussion_r2242955062
PR Review Comment: https://git.openjdk.org/jdk/pull/25450#discussion_r2242956509
PR Review Comment: https://git.openjdk.org/jdk/pull/25450#discussion_r2242958211
PR Review Comment: https://git.openjdk.org/jdk/pull/25450#discussion_r2242963383
PR Review Comment: https://git.openjdk.org/jdk/pull/25450#discussion_r2242964402
PR Review Comment: https://git.openjdk.org/jdk/pull/25450#discussion_r2242972650
PR Review Comment: https://git.openjdk.org/jdk/pull/25450#discussion_r2242976795
PR Review Comment: https://git.openjdk.org/jdk/pull/25450#discussion_r2242978435
PR Review Comment: https://git.openjdk.org/jdk/pull/25450#discussion_r2242980698
PR Review Comment: https://git.openjdk.org/jdk/pull/25450#discussion_r2242983028
PR Review Comment: https://git.openjdk.org/jdk/pull/25450#discussion_r2242985342