RFR: 8177544: Restructure G1 Full GC code

[Thomas Schatzl]

Hi,

  thanks for your hard work on the parallel full gc that starts with
this refactoring :)

On Thu, 2017-06-08 at 14:35 +0200, Stefan Johansson wrote:
> Hi,
> 
> Please review this enhancement:
> https://bugs.openjdk.java.net/browse/JDK-8177544
> 
> Webrev:
> http://cr.openjdk.java.net/~sjohanss/8177544/hotspot.00/
> 
> Summary:
> This is more or less only code moving around. The function 
> do_full_collection in G1CollectedHeap is very large and breaking it
> up to smaller parts and grouping together some of the stack objects
> help readability.
> 
> In addition to splitting the large function to smaller ones I've 
> introduced two new classes:
> - G1FullGCScope that groups most of the previously spread out stack
> objects.
> - G1SerialCollector that handles the interaction with G1MarkSweep.
> 
> Doing this change will simplify future changes to the full GC.
> 
> Testing:
> * Locally run JTREG tests
> * RBT hotspot tier 2 & 3
> 

Some initial thoughts of the change, mostly to start a discussion:

  - G1FullGCScope class: please add a line what the purpose of the
class is.

  - a better name for G1CollectedHeap::reset_card_cache_and_queue()
could be abort_refinement().

  - G1CollectedHeap.cpp:1145: please remove the "stale" word in that
comment. It's confusing me because at that point because "stale" cards
are kind of defined for a particular context and does not fit here.

  - can you move all the printing after collection
(g1CollectedHeap.cpp:1239 - 1249) into an extra method too? Something
like "print_heap_after_full_collection()"? (I think there is some
argument to also have a print_heap_before_full_collection() method).

  - G1SerialCollector is actually a "G1SerialFullCollector". I do not
remember whether the follow-up change removes it again anyway, but it
seems to be a simple renaming.

  - G1SerialCollector interface: while I could live with the
prepare/do/complete naming of the methods, the typical sequence is
(unfortunately gc_prologue(), collect(), gc_epilogue())

  - previously printing and verifying the heap has been outside the
"Pause Full" GCTraceTime. I am okay with that.

  - could we put the code from g1CollectedHeap.cpp:1215-1232 into a
"prepare_for_regular_collection" method?

  - the order of the gc_epilogue() and g1_policy-
>record_full_collection_end() calls is different.

Actually, if it were for me, I would put the whole full gc setup and
teardown into a separate class/file.

Have public gc_prologue()/collect()/gc_epilogue() methods where
gc_prologue() is the first part of do_full_collection_inner() until
application of the G1SerialCollector, collect() the instantiation and
application of G1SerialCollector, and gc_epilogue() the remainder.

E.g. in G1CollectedHeap we only have the calls to these three methods
(there is no need to have all three).

At least I think it would help a lot if all that full gc stuff would be
separate physically from do-all-G1CollectedHeap.
With the G1FullGCScope there is almost no reference to G1CollectedHeap
afaics.

(There is _allocator->init_mutator_alloc_region() call)

  - g1CollectedHeap.hpp: please try to sort the definitions of the new
methods in order of calling them.

Thanks,
  Thomas