RFR: 8367413: Refactor types in Arguments::set_heap_size() [v4]

Albert Mingkun Yang ayang at openjdk.org
Tue Oct 7 09:23:26 UTC 2025


On Tue, 7 Oct 2025 09:15:34 GMT, Joel Sikström <jsikstro at openjdk.org> wrote:

>> Hello,
>> 
>> There are several integer types used when setting the heap size ergonomically in `Arguments::set_heap_size()`, such as julong, uint64_t and size_t. It's not clear if this code works as intended on a 32-bit VM with more than 4GB physical memory. There might be issues when converting to/from size_t and uint64_t that we don't handle properly. I suggest we should be more robust and have more control over transitions between potentially smaller types. 
>> 
>> Additionally, I've gone ahead and added comments which I think makes it easier to understand what's going on in this function. I've tried my best to leave the existing behavior unchanged, apart from type conversion.
>> 
>> Testing:
>> * Oracle's tier1-8 on all Oracle supported platforms
>
> Joel Sikström has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Revert MaxRAM default removals, defer to future enhancement

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

> 1512: 
> 1513:   // Check if the user has configured any limit on the amount of RAM we may use.
> 1514:   bool ram_limit_set = !FLAG_IS_DEFAULT(MaxRAMPercentage) ||

How about `has_ram_limit`/`is_ram_limit_set`?

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

> 1542:     uint64_t max_memory = (uint64_t)(((double)physical_memory * MaxRAMPercentage) / 100);
> 1543: 
> 1544:     const size_t reasonable_min = limit_by_size_t_max(min_memory);

I wonder whether we should emit a log-info/warning, if `limit_by_size_t_max` does change the result.

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

> 1588: 
> 1589:     if (UseCompressedOops) {
> 1590:       size_t heap_end = HeapBaseMinAddress + MaxHeapSize;

I wonder if this is a pre-existing issue: `HeapBaseMinAddress` , according to its name, sounds like a `uintptr_t` instead of `size_t`.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/27224#discussion_r2409967306
PR Review Comment: https://git.openjdk.org/jdk/pull/27224#discussion_r2409970585
PR Review Comment: https://git.openjdk.org/jdk/pull/27224#discussion_r2409973796


More information about the hotspot-gc-dev mailing list