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

[Hamlin Li]

On Mon, 15 Mar 2021 13:12:15 GMT, Thomas Schatzl <tschatzl at openjdk.org> wrote:

>> Hamlin Li has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   fix compilation error
>
> 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).

Thanks Thomas, I will merge reset and initialize, and add the comment you suggested.

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

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