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

[Stefan Karlsson]

On Tue, 29 Jul 2025 10:53:16 GMT, Stefan Karlsson <stefank 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 12 commits:
>> 
>>  - 8357086: Fxied return value
>>  - 8357086: Fixed whitespaces
>>  - 8357086: Introduced usage pattern
>>  - 8357086: Fixed typo
>>  - 8357086: Refactored physical_memory in different OS
>>  - 8357086: Small fixes 2
>>  - 8357086: Small fixes 1.
>>  - 8357086: Refactored physical_memory()
>>  - 8357086: Refactored free_swap_space()
>>  - 8357086: Refactored total_swap_space()
>>  - ... and 2 more: https://git.openjdk.org/jdk/compare/75ce44aa...e4698333
>
> src/hotspot/os/bsd/os_bsd.cpp line 290:
> 
>> 288:   if (sysctl(mib, 2, &mem_val, &len, nullptr, 0) != -1) {
>> 289:     assert(len == sizeof(mem_val), "unexpected data size");
>> 290:     _physical_memory = static_cast<size_t>(mem_val);
> 
> The `__OpenBSD__` section below still uses `julong`.

It is also "interesting" that the BSD port doesn't propagate the failure to set up `_physical_memory` but the other platforms do. This adds an inconsistency across our platforms and I think that would be good to unify the behavior around that.

> src/hotspot/os/bsd/os_bsd.cpp line 1477:
> 
>> 1475:   if (!os::physical_memory(phys_mem)) {
>> 1476:     log_debug(os, thread)("os::physical_memory() failed");
>> 1477:   }
> 
> It's not clear to me that we want to have a side-effect of logging to UL here. Wouldn't it make more sense to log that physical memory info is not available to the provided `st` stream?

FWIW, I find all these UL logging statements in the os::xxx_memory functions highly dubious, and think that it would be better to have some carefully thought-through places where we log system memory information. The spread-out of the UL logging in the various memory functions look like debugging aid for HotSpot devs and not really for the users of the JVM. So, maybe they could be at most be develop logging statements. I think this could be a discussion that needs to be done outside of this PR, but since even more logging statements are added with this PR I'm bringing this up here. Could we at least skip adding these log lines in this PR? The same comment goes for the rest of the platform files.

> src/hotspot/share/gc/shared/gcInitLogger.cpp line 70:
> 
>> 68:     log_debug_p(gc, init)("os::physical_memory() failed");
>> 69:   }
>> 70:   julong memory = static_cast<julong>(phys_mem);
> 
> I would prefer to not have the extra logging in the GC code + some extra restructuring:
> 
>   size_t memory = 0;
>   if (os::physical_memory(memory)) {
>     log_info_p(gc, init)("Memory: %zu%s",
>                          byte_size_in_proper_unit(memory), proper_unit_for_byte_size(memory));
>   } else {
>     log_info_p(gc, init)("Memory: NA");
>   }

Alternatively:

  size_t memory = 0;
  (void)os::physical_memory(memory);
  log_info_p(gc, init)("Memory: %zu%s",
                       byte_size_in_proper_unit(memory), proper_unit_for_byte_size(memory));

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

PR Review Comment: https://git.openjdk.org/jdk/pull/25450#discussion_r2239481478
PR Review Comment: https://git.openjdk.org/jdk/pull/25450#discussion_r2239463594
PR Review Comment: https://git.openjdk.org/jdk/pull/25450#discussion_r2239533880