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

[Kim Barrett]

> On Mar 13, 2019, at 9:12 AM, Thomas Schatzl <thomas.schatzl at oracle.com> wrote:
> 
> Hi all,
> 
>  I fixed some time measurement related issues in a new webrev:
> 
> - do not use Tickspan.milliseconds() in measuring time because it
> truncates the result to integers :( Using "seconds() * 1000.0" as
> workaround for now, need to figure out what to do here.
> 
> - during optional evacuation the change added initial evacuation time
> too
> 
> http://cr.openjdk.java.net/~tschatzl/8218668/webrev.0_to_1/
> http://cr.openjdk.java.net/~tschatzl/8218668/webrev.1/

I've been struggling a lot with nomenclature in this review, but I
don't have a well formed description of what I'm tripping over, let
alone any concrete suggestions for changes. 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.

Despite that, I think generally a nice improvement.

------------------------------------------------------------------------------
src/hotspot/share/gc/g1/g1Policy.cpp
 662       cost_per_entry_ms = (average_time_ms(G1GCPhaseTimes::ScanRS) + average_time_ms(G1GCPhaseTimes::OptScanRS)) / (double) cards_scanned;

We have this_pause_was_young_only.  Perhaps use that to not compute
the average for OptScanRS?

------------------------------------------------------------------------------
[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.

------------------------------------------------------------------------------
src/hotspot/share/gc/g1/g1Policy.cpp
1191 void G1Policy::select_old_collection_set_regions(G1CollectionSetCandidates* candidates,
1192                                                  double time_remaining_ms,
1193                                                  uint& num_expensive_regions,
1194                                                  uint& num_initial_regions,
1195                                                  uint& num_optional_regions) {

num_expensive_regions is not used by the caller.  It is only used
locally for logging.

------------------------------------------------------------------------------
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)

------------------------------------------------------------------------------
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.

------------------------------------------------------------------------------
src/hotspot/share/gc/g1/g1CollectionSet.hpp
 171   void initialize_optional(uint max_length);

Declared, but no longer defined or used.

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