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

[Anton Artemov]

On Tue, 29 Jul 2025 11:29:54 GMT, Stefan Karlsson <stefank at openjdk.org> wrote:

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

Thanks for spotting the missed code for __OpenBSD__ section. Addressed. 

I agree that BSD behaves somewhat differently compared to other platforms from this perspective. I suggest to create a separate issue and harmonize the behavior later separately.

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

Logging removed, a proper one to be added back later.

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

Thanks, addressed.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/25450#discussion_r2242880460
PR Review Comment: https://git.openjdk.org/jdk/pull/25450#discussion_r2242883898
PR Review Comment: https://git.openjdk.org/jdk/pull/25450#discussion_r2242888685