Request for code review - 8130265: gctests/LargeObjects/large001 fails with OutOfMemoryError: Java heap space

[Jesper Wilhelmsson]

Hi Alex,

Alexander Harlap skrev den 20/8/15 21:41:
> Kim and Thomas,
>
> Here is new version with your suggestions in:
>
> http://cr.openjdk.java.net/~aharlap/8130265/webrev.03/
> <http://cr.openjdk.java.net/%7Eaharlap/8130265/webrev.03/>
> Alex
>

A few comments on this latest version.

* Why do you assert_at_safepoint and set succeeded to true in your helper? The 
original code did not do this at these places. I think you can remove these lines.

1736   assert_at_safepoint(true /* should_be_vm_thread */);
1737
1738   *succeeded = true;


* Please align the arguments in

1732 G1CollectedHeap::satisfy_failed_allocation_helper(size_t word_size,
1733                                            AllocationContext_t context,
1734                                            bool clear_all_soft_refs,
1735                                            bool* succeeded) {

and

1748   HeapWord* result = attempt_allocation_at_safepoint(word_size,
1749                                            context,
1750                                            true /* 
expect_null_mutator_alloc_region */);

and also in the header file.


* Your last version did:

attempt_allocation_at_safepoint(word_size, context, false);
expand_and_allocate(word_size, context);
do_collection(false, false, word_size);
attempt_allocation_at_safepoint(word_size, context, true);
expand_and_allocate(word_size, context);
do_collection(false, true, word_size);
attempt_allocation_at_safepoint(word_size, context, true);
assert(!collector_policy()->should_clear_all_soft_refs());
expand_and_allocate(word_size, context);

You have chosen to break this into:

attempt_allocation_at_safepoint(word_size, context, false);
expand_and_allocate(word_size, context);

satisfy_failed_allocation_helper(word_size, context, false, succeeded);
    do_collection(false, false, word_size);
    attempt_allocation_at_safepoint(word_size, context, true);
    expand_and_allocate(word_size, context);

satisfy_failed_allocation_helper(word_size, context, true, succeeded);
    do_collection(false, true, word_size);
    attempt_allocation_at_safepoint(word_size, context, true);
    expand_and_allocate(word_size, context);

assert(!collector_policy()->should_clear_all_soft_refs());

This is not entirely identical, depending on how important the assert is that 
could be an issue since the assert is now skipped in some cases where it was 
previously used.

My suggestion would be to break the code up like this:

satisfy_failed_allocation_helper(word_size, context, do_gc, clear_softrefs)
    attempt_allocation_at_safepoint(word_size, context, false);
    expand_and_allocate(word_size, context);
    do_collection(false, false, word_size);

satisfy_failed_allocation_helper(word_size, context, do_gc, clear_softrefs)
    attempt_allocation_at_safepoint(word_size, context, true);
    expand_and_allocate(word_size, context);
    do_collection(false, true, word_size);

satisfy_failed_allocation_helper(word_size, context, do_gc, clear_softrefs)
    attempt_allocation_at_safepoint(word_size, context, true);
    if (!do_gc)
       assert(!collector_policy()->should_clear_all_soft_refs());
    expand_and_allocate(word_size, context);

This would make the code in satisfy_failed_allocation() a lot more clean.


* Unless I'm missing some subtle detail here, the usage of the variable 
'succeeded' is unnecessary as 'result' will always contain the same information. 
I don't think we should pass on 'succeeded' to the helper, but simply set it 
afterwards before we return from satisfy_failed_allocation().

Thanks,
/Jesper


> On 8/20/2015 11:01 AM, Thomas Schatzl wrote:
>> Hi Alex,
>>
>> On Wed, 2015-08-19 at 18:43 -0400, Kim Barrett wrote:
>>> On Aug 19, 2015, at 5:02 PM, Alexander Harlap <alexander.harlap at oracle.com>
>>> wrote:
>>>> I agree.
>>>>
>>>> Here is new revision:
>>>>
>>>> http://cr.openjdk.java.net/~aharlap/8130265/webrev.02/
>>>> <http://cr.openjdk.java.net/%7Eaharlap/8130265/webrev.02/>
>>    would it be possible to fix the code duplication between lines
>> 1759-1784 and 1785 and 1812? The code seems to be completely the same
>> except the different flag value passed to the Full GC call and the
>> assert about that should_clear_all_soft_refs().
>>
>> Otherwise it looks good.
>>
>> Thanks,
>>    Thomas
>>
>>
>>
>