[10] RFR (M/L): 8137099: G1 needs to "upgrade" GC within the safepoint if it can't allocate during that safepoint to avoid OoME

[Thomas Schatzl]

Hi Stefan,

  thanks for your review.

On Mon, 2017-12-11 at 14:19 +0100, Stefan Johansson wrote:
> Hi Thomas,
> 
> On 2017-12-11 12:07, Thomas Schatzl wrote:
> > Hi Paul,
> > 
> >    sorry for the late reply, there has been another issue keeping
> > me
> > busy ;)
> > 
> > On Thu, 2017-12-07 at 00:44 +0000, Hohensee, Paul wrote:
> > > In attempt_allocation_humongous in g1CollectedHeap.cpp, there’s a
> > > note that says there’s code duplication between it and
> > > attempt_allocation_slow. There may be a case of version skew
> > > between the code at line 553 and the lack of similar code at line
> > > 972. If you don’t want to retry the humongous allocation attempt
> > > at that point, then the comment at line 968 should be updated.
> > 
> >    humongous allocation needs to be under the heap lock anyway, so
> > we just skip the inline allocation as it would be the same as the
> > code at the start of the loop.
> > 
> > I did add a comment, fixed some whitespace and had to move
> > declarations in the JNI c file to make it compile on some
> > platforms:
> > 
> > New webrevs:
> > http://cr.openjdk.java.net/~tschatzl/8137099/webrev.0_to_1 (diff)
> > http://cr.openjdk.java.net/~tschatzl/8137099/webrev.1 (full)
> 
> Thanks for taking on this problem. I think the change looks really
> good, just a few comments:
> ---
> g1CollectedHeap.cpp
> * The logging uses the thread pointer as identifier, I think it would
> be more useful to use the thread name. Also this one log line is
> only tagged with "gc" while the others use "gc,alloc":
> 527       log_trace(gc)(PTR_FORMAT " Unsuccessfully scheduled
> collection allocating " SIZE_FORMAT " words",
> 528                     p2i(Thread::current()), word_size);

done

> ---
> g1CollectedHeap.hpp
> * Remove "friend class VM_G1CollectForAllocation;"

Done

> * Remove/change:
>    // Callback from VM_G1CollectForAllocation operation.
>    // This function does everything necessary/possible to satisfy a
>    // failed allocation request (including collection, expansion,
> etc.)
>    HeapWord* satisfy_failed_allocation(size_t word_size,
>                                        AllocationContext_t context,
>                                        bool* succeeded);

Fine now :)

> ---
> vm_operations_g1.hpp
> * Remove "//     - VM_G1CollectForAllocation"
> 
> If you like you could also simplify the class hierarchy now, since 
> VM_G1OperationWithAllocRequest only has one sub-class.

Done.

- I changed the name of VM_G1IncCollectionPause to
VM_G1CollectForAllocation (the name of the operation that has been
deleted previously) because it is more fitting

- un-scrambled the parameters to the VM_G1CollectForAllocation
constructor

Testing with hs-tier1-5

New webrevs:
http://cr.openjdk.java.net/~tschatzl/8137099/webrev.1_to_2/ (diff)
http://cr.openjdk.java.net/~tschatzl/8137099/webrev.2/ (full)

Thanks,
  Thomas