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

Thomas Schatzl tschatzl at openjdk.java.net
Mon Mar 15 13:27:13 UTC 2021


On Fri, 12 Mar 2021 14:38:20 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:
> 
>   fix compilation error

Other than that, initial review looks good. Thanks for extracting this piece of functionality, which makes the follow-up change likely easier to review.

src/hotspot/share/gc/g1/g1FullCollector.cpp line 254:

> 252:       sum += _live_stats[j]._live_words;
> 253:     }
> 254:     _heap->set_used(sum * HeapWordSize);

`used()` is different to `live()` in that `used()` contains all space that is not allocatable (or should at least). This includes space lost due to fragmentation or just not compacting it. I think this hunk should be removed at this time until it is required.
Also, this value will be overwritten by the call to `rebuild_region_sets` in `G1FullCollector::prepare_heap_for_mutators()` as far as I can tell.

src/hotspot/share/gc/g1/g1FullGCMarker.hpp line 70:

> 68:   // Number of entries in the per-task stats entry. This seems enough to have a very
> 69:   // low cache miss rate.
> 70:   static const uint RegionMarkStatsCacheSize = 1024;

Please move this constant to `G1RegionMarkStatsCache` and reuse in both places where this constant is used instead of duplicating it.

src/hotspot/share/gc/g1/g1FullGCMarker.hpp line 84:

> 82: public:
> 83:   G1FullGCMarker(G1FullCollector* collector, uint worker_id,
> 84:                  PreservedMarks* preserved_stack, G1RegionMarkStats* mark_stats);

I think it is better at this point to put one argument per line.

src/hotspot/share/gc/g1/g1FullGCAdjustTask.cpp line 34:

> 32: #include "gc/g1/g1FullGCMarker.hpp"
> 33: #include "gc/g1/g1FullGCOopClosures.inline.hpp"
> 34: #include "gc/g1/g1RegionMarkStatsCache.inline.hpp"

This should not be needed, the cache is not accessed in this file.

src/hotspot/share/gc/g1/g1FullGCOopClosures.cpp line 30:

> 28: #include "gc/g1/g1FullGCMarker.inline.hpp"
> 29: #include "gc/g1/g1FullGCOopClosures.inline.hpp"
> 30: #include "gc/g1/g1RegionMarkStatsCache.inline.hpp"

This file does not seem to use the `G1RegionMarkStatsCache` so this include seems unnecessary.

src/hotspot/share/gc/g1/g1FullGCPrepareTask.cpp line 34:

> 32: #include "gc/g1/g1FullGCPrepareTask.hpp"
> 33: #include "gc/g1/g1HotCardCache.hpp"
> 34: #include "gc/g1/g1RegionMarkStatsCache.inline.hpp"

Not needed.

src/hotspot/share/gc/g1/g1FullGCReferenceProcessorExecutor.cpp line 31:

> 29: #include "gc/g1/g1FullGCOopClosures.inline.hpp"
> 30: #include "gc/g1/g1FullGCReferenceProcessorExecutor.hpp"
> 31: #include "gc/g1/g1RegionMarkStatsCache.inline.hpp"

Not needed.

src/hotspot/share/gc/g1/heapRegion.hpp line 30:

> 28: #include "gc/g1/g1BlockOffsetTable.hpp"
> 29: #include "gc/g1/g1HeapRegionTraceType.hpp"
> 30: #include "gc/g1/g1RegionMarkStatsCache.hpp"

This file does not use `G1RegionMarkStatsCache`, so shouldn't need an include.

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

> 47: 
> 48: // cache size is equal to or bigger than region size to intialize region_index
> 49: void G1RegionMarkStatsCache::initialize() {

If the change really wants to avoid the initial cache misses (for regions != 0), please merge this with the existing `reset()` method. I do not think it makes a difference to have this optimization that exploits the simplicity of `G1RegionMarkStatsCacheEntry::hash()`.
(And if the `hash()` implementation ever changes, this optimization won't work any more).

If you do so, also add a clear comment why the cache is initialized with these values. The comment

    // cache size is equal to or bigger than region size to intialize region_index

seems at least misplaced.

The change could, at the assignment (maybe even add an optional parameter to `clear()`) state:

    // Avoid the initial cache miss and eviction by setting the i'th's cache region_idx to the region_idx due to how the hash is calculated.

Note that resetting to constant values in that loop will likely be folded into a `memset`, while this loop needs to be translated as is, and may be a lot slower (with 100k+ regions...) in the pause.

In a textbook implementation, `G1RegionMarkStatsCacheEntry::clear()` should probably set the region index to `G1_NO_HRM_INDEX` (i.e. (uint)-1) to indicate emptiness so that all initial cache lookups fail (i.e. evict the previous, non-existing value). But then the simple implementation of the `G1RegionMarkStatsCache::evict` method will access the liveness array out of bounds... (that's probably why the initial `region_idx` values are zero, not as some sort of optimization - we save some conditional in that method).

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

Changes requested by tschatzl (Reviewer).

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



More information about the hotspot-gc-dev mailing list