RFR: JDK-8263495: Gather liveness info in the mark phase of G1 full gc [v6]

[Thomas Schatzl]

On Wed, 17 Mar 2021 02:59:30 GMT, Hamlin Li <mli at openjdk.org> wrote:

>> Gather liveness info in the mark phase of G1 full gc.
>> 
>> Per-region liveness info in the mark phase of G1 full gc is for several purposes:
>> 
>> JDK-8262068 needs it to determine whether compaction of a region should be skipped
>> JDK-8258431 for a JFR event that prints live set size estimate
>> 
>> so add this functionality.
>
> Hamlin Li has updated the pull request incrementally with one additional commit since the last revision:
> 
>   refine includes.

Some additional sweep through the code showed some more minor issues. Sorry for not noticing earlier.

src/hotspot/share/gc/g1/g1FullGCMarkTask.cpp line 63:

> 61:   // Mark stack is populated, now process and drain it.
> 62:   marker->complete_marking(collector()->oop_queue_set(), collector()->array_queue_set(), &_terminator);
> 63:   // flush live bytes to regions

Comment containing full sentences should start with upper case like the one before and end with a full stop. Also that comment is probably better placed at the declaration in the .hpp file.

src/hotspot/share/gc/g1/g1FullGCMarkTask.cpp line 64:

> 62:   marker->complete_marking(collector()->oop_queue_set(), collector()->array_queue_set(), &_terminator);
> 63:   // flush live bytes to regions
> 64:   marker->flush_mark_region_cache();

Please use `mark_stats` instead of `mark_region` in all identifier names. It is kind of jarring to have the class called `G1Mark*Stats*Cache`, all other related variables also with something containing `mark_stats` and then to use something else in the instances for this for no discernible reason.

I.e. please rename this method and the member from `_mark_region_cache` to `_mark_stats_cache`.

Of course, if you have a reason to do so please tell.

src/hotspot/share/gc/g1/g1RegionMarkStatsCache.cpp line 56:

> 54: 
> 55: // Reset liveness of all cache entries to their default values,
> 56: // initialize _region_idx to avoid initial cache miss.

Comments describing the functionality of a method should be at the declaration. Only implementation details should be added at the definition.

src/hotspot/share/gc/g1/g1RegionMarkStatsCache.hpp line 73:

> 71: 
> 72:     void clear(uint idx = 0) {
> 73:       _region_idx = idx;

I unfortunately can't comment on the other method `is_clear()` here - please remove because it is a) wrong now with that special initialization and b) not used anywhere apparently.

src/hotspot/share/gc/g1/g1RegionMarkStatsCache.hpp line 101:

> 99:   G1RegionMarkStatsCacheEntry* find_for_add(uint region_idx);
> 100: public:
> 101:   // Number of entries in the per-task stats entry. This seems enough to have a very

pre-existing: please add "value" after the "This" to read "This value seems enough to...." (or "The suggested value" or something to that effect.

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

Changes requested by tschatzl (Reviewer).

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