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

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 attemptallocationhumongous in g1CollectedHeap.cpp, there’s a >> note that says there’s code duplication between it and >> attemptallocationslow. 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.0to1 (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

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 attemptallocationslow/humongous methods (the part in "if (shouldtrygc)"), but I would like to do that in a different change.

Thanks, Thomas