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

[Alexander Harlap]

On 8/31/2015 4:40 AM, Thomas Schatzl wrote:
> Hi,
>
> On Wed, 2015-08-26 at 20:00 -0400, Alexander Harlap wrote:
>> On 8/26/2015 6:26 AM, Thomas Schatzl wrote:
>>> Hi,
>>>
>>>     looking at webrev.05, but answering here.
>>>
>>> On Tue, 2015-08-25 at 15:25 +0200, Jesper Wilhelmsson wrote:
>>>> Hi Alex,
>>>>
>>>> I like this approach :)
>>>>
>>>> Just one comment:
>>>>
>>>> After GC in the helper you put the assert that soft references should have been
>>>> handled. The first call to this helper will not clear soft references. The
>>>> original code only has this assert at the end of the function. I think it is
>>> Whatever the original code did, there is the general requirement that
>>> after any full gc, collector_policy()->should_clear_all_soft_refs()
>>> should be false.
>>>    
>>>> enough to keep it there, at the end of satisfy_failed_allocation().
>>> It seems to be randomly placed now, checking only after everything else
>>> fails. I do agree that the original code did the same, but why not
>>> improve upon this?
>>>
>>> If you want in a separate CR.
>> Yes, I would agree to make a separate CR. Only I will need help to
>> specify issue.
> Something like "Misplaced check for
> CollectedPolicy::should_clear_all_soft_refs() in G1" should be fine.
OK
>>>> 1764     assert(!collector_policy()->should_clear_all_soft_refs(),
>>>> 1765            "Flag should have been handled and cleared prior to this point");
>>>>
>>>> Besides that I'm fine with this version.
>>> - Some braces next to boolean parameters are not aligned properly. E.g.
>>> in line 1777 and 1804.
>> I am sorry, I can not locate those defects - did you mean lines 1777 and
>> 1804 in webrev.05?
> In g1CollectedHeap.cpp, line 1777 and 1804 (sorry for not mentioning the
> file):
>
> 1774   HeapWord* result =
> 1775     satisfy_failed_allocation_helper(word_size,
> 1776                                      context,
> 1777                                      true, /* do_gc */
> 1778                                      false, /* clear_all_soft_refs */
> 1779                                      false, /* expect_null_mutator_alloc_region */
> 1780                                      succeeded);
> 1781
>
> The opening comment bracket in line 1777 is not aligned to the ones
> below afaik. Same in 1804.
>
> [...]
Now I understand
>>>>>> (And the bool return value of do_collection removed).
>>>>>>
>>>>>> I also have a question about the order of
>>>>>> attempt_allocation_at_safepoint() and the expand_and_allocate(), and
>>>>>> could not find this answered before: so full gc might shrink the heap
>>>>>> too much after full gc, and we should expand the heap.
>>>>>> If I were to do this change, there would always (unconditionally) be an
>>>>>> expansion after full gc, because the current change only expands if the
>>>>>> allocation after gc fails. Also the 02 change.
>>>>>>
>>>>>> The review thread so far suggests that full gc might shrink too much,
>>>>>> but this still seems to be the case here in some cases.
>>> Could you clarify this question? To me, this looks like a potential bug
>>> that has been overlooked so far in this change.
>> Yes, full GC in some situation may over-shrink heap such way that heap
>> expansion will be needed. It was reason for
>> gctests/LargeObjects/large001 failure.  Still I prefer to do expansion
>> only after allocation attempt failed. Why should we do unconditional
>> expansion ( even if it is not needed )?
> To keep the Min/MaxHeapFreeRatio constraints the heap should observe.
>
>>   In the existing code we have
>> call to expand_and_allocate() only after
>> attempt_allocation_at_safepoint() failure. What potential bug do you mean?
> That the Min/MaxHeapFreeRatio values are not observed, causing
> performance degradation, in the worst case like in this situation.
>
> The descriptions of the flags do not indicate an exception like "do not
> care if the GC thinks it is not required".
>
> Thanks,
>    Thomas
>

Min/MaxHeapFreeRatio already were observed in calculation of shrink value. I do not see reason for expanding heap immediately after do_collection before doing attempt to allocate.