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

[Anton Artemov]

On Thu, 31 Jul 2025 08:02:17 GMT, Thomas Stuefe <stuefe at openjdk.org> wrote:

>> src/hotspot/share/runtime/arguments.cpp line 1523:
>> 
>>> 1521:   if (override_coop_limit) {
>>> 1522:     if (FLAG_IS_DEFAULT(MaxRAM)) {
>>> 1523:       (void)os::physical_memory(physical_mem_val);
>> 
>> This is one of the places where things seems to break down if we ever get a failure from os::physical_memory(). Before this patch I guess you would get random values and after this patch you will propagate 0 through this function. So, I guess it is not worse than before, but if we really can get a failure here then we do have an existing bug here.
>> 
>> With that said, when looking at this I'm wondering if there is any scenario where we can hit an error when calling the os::physical_memory? I understand that the OS APIs says that we can hit an error, but I wonder if that's only a theoretical issue? If it turns out that the APIs in effect never will fail then we can verify that they succeed during the JVM initialization and then remove the error code from os os::physical_memory() and let it only return a size_t instead. If we do this we can clean away a lot of the error handling code that is added in this PR.
>> 
>> The places that I see that could theoretically fail is:
>> 
>> 1) On AIX, but there's an `assert(0` in there to show that this really doesn't happen.
>> 
>> 2) On Windows if GlobalMemoryStatusEx(&ms) fails. The question is will this call fail for any reason on Windows? Maybe someone from Microsoft could help and tell if there ever is a reason why this code would fail:
>> 
>>   MEMORYSTATUSEX ms;
>>   ms.dwLength = sizeof(ms);
>>   GlobalMemoryStatusEx(&ms);
>
>> With that said, when looking at this I'm wondering if there is any scenario where we can hit an error when calling the os::physical_memory? I understand that the OS APIs says that we can hit an error, but I wonder if that's only a theoretical issue? If it turns out that the APIs in effect never will fail then we can verify that they succeed during the JVM initialization and then remove the error code from os os::physical_memory() and let it only return a size_t instead. If we do this we can clean away a lot of the error handling code that is added in this PR.
> 
> I absolutely agree. Keep it simple and stupid.
> 
>> 
>> The places that I see that could theoretically fail is:
>> 
>>     1. On AIX, but there's an `assert(0` in there to show that this really doesn't happen.
>> 
>>     2. On Windows if GlobalMemoryStatusEx(&ms) fails. The question is will this call fail for any reason on Windows? Maybe someone from Microsoft could help and tell if there ever is a reason why this code would fail:
>> 
>> 
>> ```
>>   MEMORYSTATUSEX ms;
>>   ms.dwLength = sizeof(ms);
>>   GlobalMemoryStatusEx(&ms);
>> ```
> 
> I don't think this can even have failed, we would have noticed:
> 
> - we use `GlobalMemoryStatusEx` to calculate `MaxRAM` (without error checking)
> - on debug builds, this would return something like "0xcccccccc_cccccccc" on error as size, since that's the default initialization by VisualC++. That's a lot of RAM :-)
> - we then base our default heap size on that. Reserving that much space for heap would have failed at at least some tests.
> 
> In product builds, the result would be random garbage; either 0, which would have failed, too, or some random number with a high probability of being very high, since the probability of random garbage that just happens to look like a small 64-bit number with zeros in the upper word is not that great. So we would have seen at least some errors on Windows in release builds, too.

Okay, makes sense, it looks like a failure of `os::physical_memory()` is something very unlikely to happen. So I made this function `void`, but, to be consistent with the rest, it provides the value by writing to the input parameter.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/25450#discussion_r2245590899