RFR: 8328168: Epsilon: 'EpsilonHeap::allocate_work' can't allocate an object larger than the uncommitted size

[Aleksey Shipilev]

On Thu, 14 Mar 2024 14:06:45 GMT, Guoxiong Li <gli at openjdk.org> wrote:

> Hi all,
> 
> When an object size meets the following three conditions, the allocation will fail unexpectedly.
> 
> 1. `object_size` > `uncommitted_size`
> 2. `object_size` > `committed_but_unused_size`
> 3. `object_size` < `unused_size` (`unused_size` equals `uncommitted_size + committed_but_unused_size`)
> 
> Please read the newly added test of this patch for the concrete case.
> 
> Thanks for taking the time to review.
> 
> Best Regrads,
> -- Guoxiong

Oh. So the critical change is here:


} else if (size_in_bytes < unused_space) {
  // No space to expand in bulk, and this allocation is still possible,
  // take all the remaining space:


Before this fix, we might have had some unused, but already expanded space. But we would miss this, since we only checked how much there was left to expand to. Which means we still have space to afford the allocations, we instead we just throw a premature OOM. Whoops.

I suggest we rename the issue to `Epsilon: Premature OOM when allocating object larger than uncommitted heap size`.

(meant to request changes)

src/hotspot/share/gc/epsilon/epsilonHeap.cpp line 129:

> 127:       size_t size_in_bytes = size * HeapWordSize;
> 128:       size_t uncommitted_space = max_capacity() - capacity();
> 129:       size_t unused_space = max_capacity() - used();

Please also: 


assert(unused_space <= uncommitted_space, 
       "Unused (" SIZE_FORMAT ") <= uncommitted (" SIZE_FORMAT ")",
       unused_space, uncommitted_space);

test/hotspot/jtreg/gc/epsilon/TestEnoughUnusedSpace.java line 31:

> 29:  * @requires vm.gc.Epsilon
> 30:  * @summary Epsilon should allocates object successfully if it has enough space.
> 31:  * @run main/othervm -XX:InitialHeapSize=64M -Xmx128M -XX:+UnlockExperimentalVMOptions

Do we really need `-XX:InitialHeapSize`, or `-Xms64M` would work as well?

test/hotspot/jtreg/gc/epsilon/TestEnoughUnusedSpace.java line 41:

> 39:         byte[] arr = new byte[90 * 1024 * 1024];
> 40:         // Just prevent the compiler optimization.
> 41:         System.out.println(arr.length);

This is also amenable for quite easy compiler optimization. See how other Epsilon tests do it: they sink the object into `static volatile Object sink;`.

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

Marked as reviewed by shade (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/18304#pullrequestreview-1937187779
Changes requested by shade (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/18304#pullrequestreview-1937226084
PR Review Comment: https://git.openjdk.org/jdk/pull/18304#discussion_r1525179207
PR Review Comment: https://git.openjdk.org/jdk/pull/18304#discussion_r1525164435
PR Review Comment: https://git.openjdk.org/jdk/pull/18304#discussion_r1525166120