RFR: 8289822: Make concurrent mark code owner of TAMSes [v2]

[Albert Mingkun Yang]

On Thu, 14 Mar 2024 09:56:02 GMT, Thomas Schatzl <tschatzl at openjdk.org> wrote:

>> Hi all,
>> 
>>   please review this change that moves TAMS ownership and handling out of `HeapRegion` into `G1ConcurrentMark`.
>> 
>> The rationale is that it's intrinsically a data structure only used by the marking, and only interesting during marking, so attaching it to the always present `HeapRegion` isn't fitting.
>> 
>> This also moves some code that is about marking decisions out of `HeapRegion`.
>> 
>> In this change the TAMSes are still maintained and kept up to date as long as the `HeapRegion` exists (basically always) as the snapshotting of the marking does not really snapshot ("copy over the `HeapRegion`s that existed at the time of the snapshot, only ever touching them), but assumes that they exist even for uncommitted (or free) regions due to the way it advances the global `finger` pointer.
>> I did not want to change this here.
>> 
>> This is also why `HeapRegion` still tells `G1ConcurrentMark` to update the TAMS in two places (when clearing a region and when resetting during full gc).
>> 
>> Another option I considered when implementing this is clearing all marking statistics (`G1ConcurrentMark::clear_statistics`) at these places in `HeapRegion` because it decreases the amount of places where this needs to be done (maybe one or the other place isn't really necessary already). I rejected it because this would add some work that is dependent on the number of worker threads (particularly) into `HeapRegion::hr_clear()`. This change can be looked at by viewing at all but the last commit.
>> 
>> Testing: tier1-4, gha
>> 
>> Thanks,
>>   Thomas
>
> Thomas Schatzl has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains six commits:
> 
>  - Merge branch 'master' into 8289822-concurrent-mark-tams-owner
>  - Do the mark information reset more fine grained (like before this change) to not potentially make HeapRegion::hr_clear() too slow
>  - Fixes
>  - some attempts to not have TAMSes always be updated for all regions
>  - Remove _top_at_mark_start HeapRegion member
>  - 8326781
>    
>    initial version
>    
>    initial draft
>    
>    some improvmeents
>    
>    Make things work
>    
>    forgotten fix

Maybe add `G1: ` in the title?

src/hotspot/share/gc/g1/g1ConcurrentMark.cpp line 592:

> 590:   if (!_g1h->collector_state()->mark_or_rebuild_in_progress()) {
> 591:     return;
> 592: }

Seem unnecessary.

src/hotspot/share/gc/g1/g1ConcurrentMark.hpp line 543:

> 541:   // Top pointer for each region at the start of marking. Must be valid for all committed
> 542:   // regions.
> 543:   HeapWord* volatile* _top_at_mark_starts;

I wonder if one can use `HeapWord* volatile _top_at_mark_starts[];` to emphasize that it's an array.

src/hotspot/share/gc/g1/g1ConcurrentMark.hpp line 569:

> 567: 
> 568:   inline HeapWord* top_at_mark_start(const HeapRegion* r) const;
> 569:   inline HeapWord* top_at_mark_start(uint region) const;

Maybe call it `region_index` or sth alike?  (In the impl of these methods, having `region` being the index is a bit confusing, IMO.)

src/hotspot/share/memory/iterator.hpp line 44:

> 42: // The following classes are C++ `closures` for iterating over objects, roots and spaces
> 43: 
> 44: class Closure : public CHeapObj<mtGC> { };

Why is this required (in this PR)?

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

PR Review: https://git.openjdk.org/jdk/pull/18150#pullrequestreview-1936235133
PR Review Comment: https://git.openjdk.org/jdk/pull/18150#discussion_r1524581727
PR Review Comment: https://git.openjdk.org/jdk/pull/18150#discussion_r1524590902
PR Review Comment: https://git.openjdk.org/jdk/pull/18150#discussion_r1524592065
PR Review Comment: https://git.openjdk.org/jdk/pull/18150#discussion_r1524599389