RFR: 8269914: Factor out heap printing for G1 young and full gc [v2]

[Albert Mingkun Yang]

On Wed, 7 Jul 2021 15:03:22 GMT, Thomas Schatzl <tschatzl at openjdk.org> wrote:

>> Hi all,
>> 
>> can I have reviews for this change that factors out before/after heap related printing into a single scoped object to be used for both young and full gc code?
>> 
>> There is some minor change in order of printing of unrelated log output (COMMIT/UNCOMMIT region state changes vs. other heap printing), during full gc but since this is very low level debugging I do not see a problem to do that.
>> 
>> (Sorry for the trouble with open/closing this issue already: have been experimenting with the feature to base PRs on other PRs which kind-of failed a bit; rebased on tip after being able to push all dependencies, so this one should be good)
>> 
>> To keep JFR event sending in the correct order, this change includes JDK-8270018.
>> 
>> Thanks,
>>   Thomas
>
> Thomas Schatzl has refreshed the contents of this pull request, and previous commits have been removed. The incremental views will show differences compared to the previous content of the PR.

Many comments are very subjective; the current version looks fine as well.

src/hotspot/share/gc/g1/g1CollectedHeap.cpp line 983:

> 981: };
> 982: 
> 983: void G1CollectedHeap::print_heap_after_full_collection() {

It's a bit odd to see `*_full_collection` as an API of `G1CollectedHeap`; I would have expected full-collection-only APIs in g1 full gc files.

src/hotspot/share/gc/g1/g1CollectedHeap.cpp line 2948:

> 2946:   _g1h->print_heap_after_gc();
> 2947:   // We are at the end of the GC. Total collections has already been increased.
> 2948:   _g1h->rem_set()->print_periodic_summary_info("After GC RS summary", _g1h->total_collections() - 1);

It's unclear to me why they are separated into two sections here. Additionally, does the order matter? If not, is it possible to group the two `rem_set()` calls together?

src/hotspot/share/gc/g1/g1CollectedHeap.cpp line 2981:

> 2979:   void report_pause_type(G1GCPauseType type) {
> 2980:     tracer()->report_young_gc_pause(type);
> 2981:   }

This one seems unused.

src/hotspot/share/gc/g1/g1CollectedHeap.cpp line 3070:

> 3068:         _allocator->release_mutator_alloc_regions();
> 3069: 
> 3070:         calculate_collection_set(*jtm.evacuation_info(), target_pause_time_ms);

`*jtm.evacuation_info()` seems odd; `calculate_collection_set` could use pointer instead of reference, right?

src/hotspot/share/gc/g1/g1FullGCScope.hpp line 71:

> 69:                 bool clear_soft,
> 70:                 bool do_maximal_compaction);
> 71:   ~G1FullGCScope() { }

Why not just remove this destructor?

-------------

Changes requested by ayang (Committer).

PR: https://git.openjdk.java.net/jdk/pull/4705