CRR (M): 7132029: G1: mixed GC phase lasts for longer than it should

[Bengt Rutisson]

Hi Tony,

I really like this cleanup! Thanks for fixing it.

Some comments:

collectionSetChooser.hpp:

_numMarkedRegions is kind of a confusing name to me. It does not contain 
all the marked regions. It contains all the old regions that we would 
like to include in the collection set. How about _num_selected_regions 
or even _num_selected_old_regions?

I think the remove(HeapRegion* hr) method is kind of strange. I 
understand the need for peek() from finalize_cset(), but I think it is 
strange that the remove() dependes on the code first calling peek(). I'd 
prefer if we could make this more iterator style or at least change 
remove() to someting like pop() that will return the next HeapRegion 
instead of taking it as a parameter.


collectionSetChooser.cpp:

Why is the method CollectionSetChooser::updateAfterFullCollection() 
needed? Can't G1CollectorPolicy::record_full_collection_end() do 
_collectionSetChooser->clearMarkedHeapRegions() just like 
G1CollectorPolicy::record_concurrent_mark_cleanup_end() does?


g1_globals.hpp:

It seems to me that at least the G1MaxMixedGCNum and the 
G1OldCSetRegionThresholdPercent can be things that end users might need 
to tweak until we have the heuristics more thoroughly tested out. Would 
it be ok to make them available in product builds? Maybe as 
experiemental flags?


g1CollectorPolicy.cpp:

       if (next_gc_should_be_mixed("end of young GC")) {
         ergo_verbose0(ErgoMixedGCs,
                       "start mixed GCs",
                       ergo_format_reason("appropriate old regions 
available"));
         set_gcs_are_young(false);
       } else {
         ergo_verbose0(ErgoMixedGCs,
                       "do not start mixed GCs",
                       ergo_format_reason("appropriate old regions 
available"));
       }

I think the ergo_verbose message in the else case at lines 1490-1492 
should say "appropriate old regions not available", right?

We log the ergo decisions in next_gc_should_be_mixed(). Will it not be 
duplicate information to log it in 
G1CollectorPolicy::record_collection_pause_end() as well?

Bengt


On 2012-02-14 16:18, Tony Printezis wrote:
> Hi all,
>
> I'd like a couple of (quick please!) code reviews for this change:
>
> http://cr.openjdk.java.net/~tonyp/7132029/webrev.0/
>
> The policy that was choosing old regions for collection during mixed 
> GCs was buggy and it could exhibit many strange behaviors, one of 
> which was to go into mixed GC mode, do "mixed GCs" (which they did not 
> actually collect any old regions), and not get out of it for a while 
> while preventing subsequent marking cycles not to start. This 
> frequently caused evac failures and Full GCs.
>
> The logic on which old regions to add to the CSet was spread to many 
> places. I simplified that and put it mainly in the loop in the 
> finalize_cset() method (renamed from choose_collection_set(), 
> "finalize" is more descriptive on what the method does). I think the 
> new version is easier to follow.
>
> Description of the changes:
>
> * I have introduced min and max number of old regions to be added to 
> the CSet of each mixed GC. Max number is calculated as a percentage 
> over the heap size (default: 10% - thanks to Jesper for suggesting to 
> use a heap percentage for this) and ensures that collections will not 
> get super long. Min number is calculated based on a desired mixed GC 
> num after a marking cycle (default: 4) and ensures that each mixed GC 
> will make some progress in collecting old regions (so that the 
> candidate old regions are collected in a timely fashion).
>
> * We now don't add any regions with live percentage over a threshold 
> (default: 95%) to the CSet chooser and we do not consider them for 
> collection.
>
> * I stopped using the cache in the CSet chooser class (it was used to 
> resort regions according to their latest GC efficiency), since it's 
> not necessary any more: we'll now go through all the candidate old 
> regions in the CSet chooser class and we don't have a heuristic of 
> when to stop mixed GCs based on GC efficiency.
>
> * I introduced the notion of "reclaimable bytes" on HeapRegions, which 
> not only includes the predicted garbage bytes on each region, but also 
> the unused space in the area [top,end) which will also be reclaimed. 
> The CSet chooser class now keeps track of the total reclaimable bytes 
> of all the regions that it tracks. If that falls under a certain 
> threshold (default: 1% of the heap) we stop doing mixed GCs as we'll 
> reclaim very little out of the remaining candidate old regions.
>
> * I eliminated the case where a mixed GC starts and picks no old 
> regions to collect (I hope!). Now the information on the CSet chooser 
> (remaining region num / remaining reclaimable bytes) can tell us 
> whether we want to do more mixed GCs or not. If we do, it's guaranteed 
> that there will be old regions to collect. Because of that, I removed 
> the _should_revert_to_young_gcs flag as it's not needed any more. It 
> was used so that the CSet choice code could flag that we should stop 
> doing mixed GCs at the end of the GC. Now, we can decide that by just 
> looking at the information on the CSet chooser (the heuristic is 
> encapsulated in the next_gc_should_be_mixed()).
>
> * I also changed the policy in the case where a fixed young gen is 
> used. Before, we were a bit arbitrary: during mixed GCs we'd cut the 
> young gen size in half and fill up the rest with old regions. I 
> thought that trying to re-use the "desired mixed GC number" heuristic 
> for this made sense. So, I now add the min number of old regions to 
> each mixed GC (as long as we don't go over the max). I think this is a 
> more reasonable and less arbitrary heuristic to what we had before and 
> it's more consistent with the non-fixed young gen policy. I didn't 
> know whether I should decrease the young gen size for each mixed GC, 
> like how we did before, and by how much. So, I now leave it unchanged 
> (the user decided they want a fixed young gen, they will now always 
> get it!).
>
> * Updated all related ergo output to print the new information.
>
> Like the marking changes, this change leaves a fair amount of unused 
> code that we can remove. I had to draw the line somewhere on how much 
> I should remove now and how much I should leave for later. I opened a 
> new CR for the remaining cleanup:
>
> 7145441: G1: collection set chooser-related cleanup
>
> Many thanks to Charlie for doing some last-minute performance testing 
> with my workspace. He was the one who discovered the problem with the 
> never-ending mixed GCs and this fix eliminated the problem. 
> Correctness-testing-wise, I've run overnight with the GC test suite.
>
> Thanks,
>
> Tony
>
>