RFR: 8177544: Restructure G1 Full GC code

[Stefan Johansson]

Thanks Thomas for reviewing,

On 2017-06-13 11:36, Thomas Schatzl wrote:
> 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.
Added a sentence, do you want more?
>    - a better name for G1CollectedHeap::reset_card_cache_and_queue()
> could be abort_refinement().
Sounds good, fixed.
>
>    - 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.
Fixed.
>    - 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).
Done, since this needed me to pass around the heap_transition I decided 
to move it into the G1FullGCScope and I think that was an improvement in 
it self.
>    - 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.
Yes, it will be removed. And yes I can do the rename.
>
>    - 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())
I'm a bit hesitant about re-using or gc_*logue and moving stuff into 
them if that's what you mean. And if you can live with the current 
proposal I think I will stick with it.
>
>    - previously printing and verifying the heap has been outside the
> "Pause Full" GCTraceTime. I am okay with that.
I see, and I also think the new way is ok.
>
>    - could we put the code from g1CollectedHeap.cpp:1215-1232 into a
> "prepare_for_regular_collection" method?
Yes, will group them together and also include the above assert. I'll 
also move the MemoryService::track_memory_usage() call into gc_epilogue 
as it is called at a similar point for the YC. I called the new method 
prepare_heap_for_mutators.
>
>    - the order of the gc_epilogue() and g1_policy-
>> record_full_collection_end() calls is different.
The reason I moved them around is that 
increment_old_marking_cycles_completed has been moved into the epilogue. 
I was uncertain if the policy needed to see that update before recording 
the end. Digging into the policy I think this is not the case, I'll 
reorder them again.
> 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)
I see your point and I think it would be good. But as we discussed over 
chat, might be something to look at once everything else in this area is 
done. Will create a RFE for this.

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

Here are updated webrevs:
Full: http://cr.openjdk.java.net/~sjohanss/8177544/hotspot.01/
Inc: http://cr.openjdk.java.net/~sjohanss/8177544/hotspot.00-01/

Thanks,
Stefan
>
> Thanks,
>    Thomas