RFR: 8318817: Could not reserve enough space in CodeHeap 'profiled nmethods' (0K) [v4]

[Thomas Stuefe]

On Tue, 31 Oct 2023 17:33:48 GMT, Daniel Lundén <duke at openjdk.org> wrote:

>> This changeset fixes an issue where certain large values of `-XX:NMethodSizeLimit` could cause code heap initialization to fail with the error message
>> 
>> Error occurred during initialization of VM
>> Could not reserve enough space in CodeHeap 'profiled nmethods' (0K)
>> 
>> 
>> Two components in `CodeCache::initialize_heaps` may cause the error:
>> 1. The minimum size (`min_size = os::vm_page_size();`) for profiled and non-profiled code heaps is sometimes not large enough. Specifically, when `profiled_size` and/or `non_profiled_size` are set to `min_size` and `alignment > os::vm_page_size()`, the `align_down` below (from `CodeCache::initialize_heaps`) sets the size to 0.
>>       ```
>>       const size_t alignment = MAX2(ps, os::vm_allocation_granularity());
>>       non_nmethod_size = align_up(non_nmethod_size, alignment);
>>       profiled_size    = align_down(profiled_size, alignment);
>>       non_profiled_size = align_down(non_profiled_size, alignment);
>>       ```
>> 2. The calculation for default code heap sizes does not consider the edge case when `cache_size > non_nmethod_size > cache_size - 2 * min_size`.
>> 
>> This changeset
>> 1. ensures that `min_size >= alignment`, and
>> 2. fixes the edge case.
>> 
>> The changeset includes new tests that exercise the above, also in combination with `-XX:+UseTransparentHugePages` on Linux  to test the case when `os::can_execute_large_page_memory()` is true (which can affect `alignment`).
>> 
>> ### Testing (on all Oracle-supported platforms)
>> `tier1`, `tier2`, `tier3`, `tier4`, `tier5`
>
> Daniel Lundén has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Invert incorrect assert

Some comments, otherwise good.

src/hotspot/share/code/codeCache.cpp line 204:

> 202:   bool non_profiled_set     = FLAG_IS_CMDLINE(NonProfiledCodeHeapSize);
> 203:   const size_t ps           = page_size(false, 8);
> 204:   const size_t min_size     = MAX2(os::vm_allocation_granularity(), ps);

There is no need for this, nor for the assert below. We know what these sizes are, and their relationship to each other. 

AIX: ps <= os::vm_page_size == alloc granularity
Windows: ps == os::vm_page_size < alloc granularity
Everyone else: ps == os::vm_page_size == alloc granularity

If it weren't so, a lot of things would break. Just use alloc granularity.

src/hotspot/share/code/codeCache.cpp line 241:

> 239:       // Ensure at least min_size is available for profiled and non-profiled
> 240:       // methods
> 241:       non_nmethod_size = MIN2(non_nmethod_size, cache_size - 2 * min_size);

Would it not be simpler to just do above:


if (cache_size > (non_method_size - 2 * min_size)) {


?

Also, can this calculation produce 0 or negative overflow?

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

PR Review: https://git.openjdk.org/jdk/pull/16373#pullrequestreview-1706918370
PR Review Comment: https://git.openjdk.org/jdk/pull/16373#discussion_r1377964374
PR Review Comment: https://git.openjdk.org/jdk/pull/16373#discussion_r1377992225