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

[Thomas Schatzl]

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.

> >
> >> 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.

[...]
> >>>>
> >>>> (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