RFR (XL): 8218668: Clean up evacuation of optional collection set

[Kim Barrett]

> On Apr 4, 2019, at 7:18 AM, Thomas Schatzl <thomas.schatzl at oracle.com> wrote:
> On Tue, 2019-04-02 at 21:02 -0400, Kim Barrett wrote:
>>> I think it might help if
>> there was a term for the non-optional part of the collection set; a
>> point of confusion for me is whether something is about that or the
>> complete collection set.
> 
> They are called "initial old regions" in the code, however they do not
> play a huge role.

Not what I was referring to, but I think not important now.  The changes
mentioned below helped a lot.

> There were some outdated comments that might have confused you :( I
> updated the webrev with better comments and hopefully improved and more
> conistent naming.
> 
> There is now also a huge comment in g1CollectionSet.hpp that gives an
> overview of how the collection set looks like and changes over time,
> with examples.

Thanks, much improved.  One minor nit:

------------------------------------------------------------------------------
src/hotspot/share/gc/g1/g1CollectionSet.hpp
  43 // The set of regions that were evacuated during an evacuation pause.

"were evacuated" seems the wrong tense here.  Maybe "are evacuated"?

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

>> [pre-existing] adaptive_young_list_length() seems like a bad name for
>> a predicate.  Maybe add "use_" prefix or something like that?  This
>> could be a separate RFE.
> 
> I will fix this separately.

OK.

>> src/hotspot/share/gc/g1/g1Policy.cpp
>> 1191 void
>> G1Policy::select_old_collection_set_regions(G1CollectionSetCandidates
>> * candidates,
>> ...
>> 
>> I'm wondering if things might be simpler if the API here was instead
>> 
>>  select_old_collection_set_regions(collection_set, candidates,
>> time_remaining)
>> 
>> (where I'm not sure "select" is the right word)
> 
> Renamed to "calculate"; so you suggest that the policy immediately puts
> the results of that method into the collection set?
> 
> I thought it would be easier to understand if, like the current code
> does, just calculates which regions from the candidate set we are going
> to process in this collection (i.e. the "initial old regions" and the
> "optional old regions") given the time limit, and clearly separate that
> from doing the legwork to add these to the correct sets and do other
> preparation.

I was thinking it *might* be simpler to have a loop that looks something like

  while there are more candidates and next candidate fits
    transfer next candidate

But what’s there is okay.   

>> src/hotspot/share/gc/g1/g1CollectedHeap.hpp
>> 745   // Evacuate the next set of optional regions.
>> 746   void evacuate_next_optional_regions(G1ParScanThreadStateSet*
>> per_thread_states);
>> 
>> This appears to be a helper function and shouldn't be public.
> 
> Fixed. I moved another one or two methods to private.

Thanks for the further privatizations.

> 
> New webrevs:
> http://cr.openjdk.java.net/~tschatzl/8218668/webrev.1_to_2/
> http://cr.openjdk.java.net/~tschatzl/8218668/webrev.2/
> 
> New webrevs:
> http://cr.openjdk.java.net/~tschatzl/8218668/webrev.2_to_3/
> http://cr.openjdk.java.net/~tschatzl/8218668/webrev.3/ 

Other than the one wording mentioned above, looks good.  I don’t need a new
webrev for that.