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

[Thomas Schatzl]

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/

Thanks,
  Thomas

On Mon, 2019-03-04 at 12:56 +0100, Thomas Schatzl wrote:
> Hi all,
> 
>   please have a look at the following change that fixes a few
> (overall minor) problems with the Abortable Mixed GC implementation
> introduced in JDK12.
> 
> Mainly, the collection of the optional collection set has been kind-
> of tacked on as a completely separate thing with its own collection
> set, its own iteration loop etc.
> 
> This change integrates the optional evacuation much better imo into
> the existing program flow.
> 
> Automatically it fixes a few issues with the existing implementation
> that were mostly already found and decided to be benign enough to
> move to another (this) stage (from the CR):
> 
> - the optional collection set evacuation duplicates quite a bit of
> "first pass" evacuation code while conceptually very similar. This
> should be unified and cleaned up as much as possible. 
> - with the optional collection set the code creates copies of parts
> of the candidate collection set, first popping it from it and then
> pushing back leftovers. This is not necessary, an index (cursor)
> within the candidate collection set is sufficient. 
> 
> - if there is an evacuation failure during evacuating an optional
> part, we try to fix up self-looped oops for the entire collection set
> which is not necessary. Obviously only regions part of the current
> collection pass need to be looked at. 
> - the work distribution when evacuating an optional collection set is
> bad: every thread processes the same regions in the same sequence 
> - calculation of total number of cards scanned and total scan RS time
> does not take optional evacuation into account 
> - optional collection set log does not show code roots time (from the
> heap region's remembered sets) and termination time completely 
> - optional collection also does not add "code root fixup time" 
> - optional collection does not add to the "par time" (which is
> actually the time for the parallel evacuation), which means that the
> calculation for the "Other" time is wrong (result too high) impacting
> prediction (being too conservative) 
> - the opt_index_in_cset of a heap region is not reset for all regions
> every time after GC. Since it is properly overwritten as needed this
> seems benign.
> 
> Conceptually, nothing really changed: G1 young gc performs evacuation
> of the "initial collection set", i.e. young gen + an initial set of
> old gen regions (mixed gc only), and after that an incremental
> collection of optional regions as before (mixed gc only). However I
> tried to improve naming to work that out a bit better.
> 
> Originally I wanted to make the code even more integrated,
> particularly merge the two gangtasks to evacuate the initial
> collection set and the optional collection set; however I could not
> find a good way due to the necessary MarkScopes.
> 
> Previous recent changes were basically preparations for this change
> that were split out, but I strongly recommend looking at "JDK-
> 8219100: Improve do_collection_pause_at_safepoint" also out for
> review right now before this change as it moves around quite a bit of
> code, and you might feel a bit lost with the baseline code otherwise.
> 
> CR:
> https://bugs.openjdk.java.net/browse/JDK-8218668
> Webrev:
> http://cr.openjdk.java.net/~tschatzl/8218668/webrev/
> Testing:
> hs-tier1-8; no performance differences
> 
> Thanks,
>   Thomas
> 
>