RFR: 8213890: Implementation of JEP 344: Abortable Mixed Collections for G1

[Stefan Johansson]

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::initialize_optional(uint max_length) {
> 
> Consider also asserting _option_region_length == 0.
Fixed, also added assert for _optional_region_max_length == 0.

> 
> ------------------------------------------------------------------------------
> src/hotspot/share/gc/g1/g1CollectionSet.cpp
>   451 bool G1CollectionSet::optional_is_full() {
> 
> Consider asserting _optional_region_length <= _option_region_max_length.
Good point, added.

> 
> ------------------------------------------------------------------------------
> src/hotspot/share/gc/g1/g1ParScanThreadState.cpp
>    40 G1ParScanThreadState::G1ParScanThreadState(G1CollectedHeap* g1h,
> ...
>    86   _oops_into_optional_regions = NEW_C_HEAP_ARRAY(G1OopStarChunkedList, _num_optional_regions, mtGC);
>    87   for (size_t i = 0; i < _num_optional_regions; i++) {
>    88     ::new (_oops_into_optional_regions + i) G1OopStarChunkedList();
>    89   }
> 
> Why not something like
> 
>    _oops_in_optional_regions = new G1OopStarChunkedList[_num_optional_regions];
>
I agree this is nicer, and I assume the memory will be associated with 
mtGC since the G1OopStarChunkedList is CHeapObj<mtGC>.

> and the corresponding delete[] in ~G1ParScanThreadState().
> 
> This would require adding ~G1OopStarChunkedList to free the chunk
> lists (possibly eliminating the need for public free_chunk_lists). It
> probably also requires giving G1OopStarChunkedList some mechanism for
> getting the "used_by_optional" 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 _index_in_opt_cset;
>   256   int  _young_index_in_cset;
> 
> It's surprising (and somewhat disturbing) that the type of the new
> _index_in_opt_cset different from the type of the pre-existing
> _young_index_in_cset. 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 UINT_MAX as the "undefined" value.) I wonder if
> _young_index_in_cset 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.

> 
> ------------------------------------------------------------------------------
>