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