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

[Thomas Schatzl]

Hi Kim,

On Tue, 2019-04-02 at 21:02 -0400, Kim Barrett wrote:
> > 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.

They are called "initial old regions" in the code, however they do not
play a huge role.

Let me digress about the naming in the code which I tried to adhere
to: 

- the collection set consists of "young part" and "old part"
- the "young part" consists of eden and survivor regions (which are
relabeled as eden almost immediately).
- the "old part" itself consists of "initial old regions" and "optional
[old] regions".
- the "initial collection set" consists of "young part" and "initial
old regions", collected by the "initial evacuation" phase.
- the "optional collection set" is a set of increments over the
"optional old regions" that is collected in the "optional evacuation"
phase.

However I found it better to think of increments of the collection set,
the parts the next evacuation will work on. Then there is only the
"current increment" and "all" parts of the collection set to think
about than to worry about what it is actually in a particular increment
as the actual copying handles them completely the same.

For the "initial evacuation" phase, the increment is survivor + eden +
initial old regions, for the "optional evacuation" phase, which is a
sequence of evacuations of parts over the "optional old regions", each
part being the increment that is currently worked on.

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.

> 
> Despite that, I think generally a nice improvement.
> 

Thanks.

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

I changed the code to only calculate the average if the collection had
been a mixed collection. Originally I simply did not see a performance
problem with that.

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

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

Removed. Oversight from moving code around...

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

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

> 
> -------------------------------------------------------------------
> -----------
> src/hotspot/share/gc/g1/g1CollectionSet.hpp
>  171   void initialize_optional(uint max_length);
> 
> Declared, but no longer defined or used.

Removed.

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

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

Thanks,
  Thomas