RFR: 8328168: Epsilon: Premature OOM when allocating object larger than uncommitted heap size [v2]

[Guoxiong Li]

On Thu, 14 Mar 2024 16:32:57 GMT, Aleksey Shipilev <shade at openjdk.org> wrote:

>> Guoxiong Li has updated the pull request incrementally with 32 additional commits since the last revision:
>> 
>>  - Adjust test.
>>  - Add assert statement.
>>  - Merge branch 'master' into FAILED-ALLOCATION
>>  - 8328228: Missing comma in copyright year for a few JColorChooser tests
>>    
>>    Reviewed-by: jpai
>>  - 8328130: Remove applet usage from JColorChooser tests Test4759934
>>    
>>    Reviewed-by: prr
>>  - 8328121: Remove applet usage from JColorChooser tests Test4759306
>>    
>>    Reviewed-by: azvegint
>>  - 8327857: Remove applet usage from JColorChooser tests Test4222508
>>    
>>    Reviewed-by: prr, tr
>>  - 8327859: Remove applet usage from JColorChooser tests Test4319113
>>    
>>    Reviewed-by: prr, tr
>>  - 8328011: Convert java/awt/Frame/GetBoundsResizeTest/GetBoundsResizeTest.java applet test to main
>>    
>>    Reviewed-by: honkar, aivanov
>>  - 8328021: Convert applet test java/awt/List/SetFontTest/SetFontTest.html to main program
>>    
>>    Reviewed-by: honkar, abhiscxk
>>  - ... and 22 more: https://git.openjdk.org/jdk/compare/4e1e36d8...5c99942d
>
> 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);

I think it should be `assert(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?

Changed to `-Xms64M`.

> This is also amenable for quite easy compiler optimization.

You are right, my mistake.The length of this array can be statically determined.

>  See how other Epsilon tests do it: they sink the object into `static volatile Object sink;`.

Fixed.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/18304#discussion_r1525776994
PR Review Comment: https://git.openjdk.org/jdk/pull/18304#discussion_r1525776936
PR Review Comment: https://git.openjdk.org/jdk/pull/18304#discussion_r1525776962