RFR: 8213890: Implementation of JEP 344: Abortable Mixed Collections for G1 (original) (raw)

Stefan Johansson stefan.johansson at oracle.com
Tue Nov 27 19:36:14 UTC 2018

Thanks for the review Kim,

Two new webrevs, I'll let you decide which way to go, I kind of prefer version b. Full a: http://cr.openjdk.java.net/~sjohanss/8213890/03a/ Inc a: http://cr.openjdk.java.net/~sjohanss/8213890/02-03a/ Full b: http://cr.openjdk.java.net/~sjohanss/8213890/03b/ Inc b: http://cr.openjdk.java.net/~sjohanss/8213890/02-03b/

Comments below.

On 2018-11-27 03:20, Kim Barrett wrote:

On Nov 26, 2018, at 4:03 PM, Stefan Johansson <stefan.johansson at oracle.com> wrote:

Thanks Thomas, Updated webrevs Full: http://cr.openjdk.java.net/~sjohanss/8213890/02/ Inc: http://cr.openjdk.java.net/~sjohanss/8213890/01-02/ Looks good. Just a few very minor things, which you can do or not. ------------------------------------------------------------------------------ src/hotspot/share/gc/g1/g1CollectionSet.cpp 105 void G1CollectionSet::initializeoptional(uint maxlength) { Consider also asserting optionregionlength == 0. Fixed, also added assert for _optional_region_max_length == 0.

------------------------------------------------------------------------------ src/hotspot/share/gc/g1/g1CollectionSet.cpp 451 bool G1CollectionSet::optionalisfull() { Consider asserting optionalregionlength <= optionregionmaxlength. Good point, added.

------------------------------------------------------------------------------ src/hotspot/share/gc/g1/g1ParScanThreadState.cpp 40 G1ParScanThreadState::G1ParScanThreadState(G1CollectedHeap* g1h, ... 86 oopsintooptionalregions = NEWCHEAPARRAY(G1OopStarChunkedList, numoptionalregions, mtGC); _87 for (sizet i = 0; i < numoptionalregions; i++) {_ _88 ::new (oopsintooptionalregions + i) G1OopStarChunkedList();_ _89 }_ _Why not something like_ _oopsinoptionalregions = new G1OopStarChunkedList[numoptionalregions];_ I agree this is nicer, and I assume the memory will be associated with mtGC since the G1OopStarChunkedList is CHeapObj.

and the corresponding delete[] in ~G1ParScanThreadState().

This would require adding ~G1OopStarChunkedList to free the chunk lists (possibly eliminating the need for public freechunklists). It probably also requires giving G1OopStarChunkedList some mechanism for getting the "usedbyoptional" information.

Yes, the public free_chunk_lists() was added for this reason, previously we explicitly called the destructor and had the freeing in there. Doing the above change I consider keeping the free_chunk_lists() method but moving it to the end of the optional phase, to free the memory when we are done with it. I also made a version b) which instead accumulate the used memory in an instance variable.

When doing that I realized there is a bug with the current metrics, if we do more than one call to evacuate_optional_regions (which can happen if we evac faster than predicted) we will overwrite the metrics (or assert in debug builds). So this fix grew a bit.

------------------------------------------------------------------------------ src/hotspot/share/gc/g1/heapRegion.hpp 253 // The index in the optional regions array, if this region 254 // is considered optional during a mixed collections. 255 uint indexinoptcset; 256 int youngindexincset; It's surprising (and somewhat disturbing) that the type of the new indexinoptcset different from the type of the pre-existing youngindexincset. It seems to be correct, but unpleasant. I'm guessing you get fewer annoying casts in the new code by doing it this way? (With UINTMAX as the "undefined" value.) I wonder if youngindexincset would benefit from similar treatment. Maybe an RFE? Yes, something needs to be done here, my preferred way (I think) would be to merge the two. I looked a bit at this but there are some problems with that. I will file an RFE once everybody is fine with this change going in as is.

------------------------------------------------------------------------------

More information about the hotspot-gc-dev mailing list