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

[Stefan Johansson]

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);
---
g1CollectedHeap.hpp
* Remove "friend class VM_G1CollectForAllocation;"
* 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);
---
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.
---

Thanks,
Stefan

> There is some potential to do refactoring in the
> attempt_allocation_slow/humongous methods (the part in "if
> (should_try_gc)"), but I would like to do that in a different change.
>
> Thanks,
>    Thomas
>