RFR: 8367485: os::physical_memory is broken in 32-bit JVMs when running on 64-bit OSes [v2]

[Anton Artemov]

On Thu, 18 Sep 2025 13:03:48 GMT, Stefan Karlsson <stefank at openjdk.org> wrote:

>> Anton Artemov has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   8367485: Refactoring with uint64_t
>
> src/hotspot/os/linux/os_linux.cpp line 306:
> 
>> 304:     return false;
>> 305:   }
>> 306:   value = static_cast<uint64_t>(si.totalswap * si.mem_unit);
> 
> Could the `si.totalswap * si.mem_unit` calculation overflow on 32-bit JVMs? I wonder if it would be safer to write the code this way:
> 
> static_cast<uint64_t>(si.totalswap) * si.mem_unit;
> 
> The same comment goes for si.freeswap below.

Yes it can, addressed as suggested.

> src/hotspot/os/linux/os_linux.cpp line 2572:
> 
>> 2570:   struct sysinfo si;
>> 2571:   sysinfo(&si);
>> 2572:   uint64_t phys_mem = physical_memory();
> 
> You need to update the two `%zu` below.

Right, somehow missed these two.

> src/hotspot/os/windows/os_windows.cpp line 3967:
> 
>> 3965: // Processor level is not available on non-NT systems, use vm_version instead
>> 3966: int    os::win32::_processor_level           = 0;
>> 3967: uint64_t os::win32::_physical_memory           = 0;
> 
> The indentation is misaligned now.

Addressed.

> src/hotspot/os/windows/os_windows.hpp line 43:
> 
>> 41:   static int    _processor_type;
>> 42:   static int    _processor_level;
>> 43:   static uint64_t _physical_memory;
> 
> Indentation.

I changed indentation of all members around, is that what you mean?

> src/hotspot/share/gc/z/zLargePages.cpp line 34:
> 
>> 32:   pd_initialize();
>> 33: 
>> 34:   const uint64_t memory = os::physical_memory();
> 
> PROPERFMT below uses `%zu`. Given that ZGC is only compiled to 64-bits maybe you could make the memory variable size_t here. I don't know if you need an explicit cast or not.

I prefer having explicit casting to avoid surprises in the future. Addressed.

> src/hotspot/share/jfr/periodic/jfrPeriodic.cpp line 534:
> 
>> 532:   EventPhysicalMemory event;
>> 533:   event.set_totalSize(totalPhysicalMemory);
>> 534:   uint64_t avail_mem = 0;
> 
> I think you can remove all usages of u8 in this function.

Yes provided that u8 will always be uint64_t, which may or may not be true in the future. Removed as suggested.

> src/hotspot/share/runtime/os.cpp line 1948:
> 
>> 1946:   /* Is this a server class machine? */
>> 1947:   if ((os::active_processor_count() >= (int)server_processors) &&
>> 1948:       (phys_mem >= static_cast<uint64_t>(server_memory - missing_memory))) {
> 
> You don't need the cast if you change the type of `server_memory` and `missing_memory` to uint64_t instead.

Good point! Addressed.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/27335#discussion_r2359400370
PR Review Comment: https://git.openjdk.org/jdk/pull/27335#discussion_r2359402435
PR Review Comment: https://git.openjdk.org/jdk/pull/27335#discussion_r2359400620
PR Review Comment: https://git.openjdk.org/jdk/pull/27335#discussion_r2359401008
PR Review Comment: https://git.openjdk.org/jdk/pull/27335#discussion_r2359401382
PR Review Comment: https://git.openjdk.org/jdk/pull/27335#discussion_r2359401748
PR Review Comment: https://git.openjdk.org/jdk/pull/27335#discussion_r2359402021