RFR: 8276887: G1: Move precleaning to Concurrent Mark From Roots subphase [v3]

[Kim Barrett]

On Wed, 10 Nov 2021 21:09:10 GMT, Albert Mingkun Yang <ayang at openjdk.org> wrote:

>> Simple change of moving the "Preclean" subphase into "Mark from roots" subphase so that precleaning becomes parallel automatically. Evaluation using a contrived java program shows that multiple GC threads do precleaning. More detailed testing results are available in the JBS ticket.
>> 
>> Test: hotspot_gc
>
> Albert Mingkun Yang has updated the pull request incrementally with two additional commits since the last revision:
> 
>  - review
>  - Revert "worker_id"
>    
>    This reverts commit 0841db5c5c2b39372058b6ba0d8027aae3716669.

Changes requested by kbarrett (Reviewer).

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

> 910:   ReferenceProcessor*   _cm_rp;
> 911: 
> 912:   void do_mark_step(G1CMTask* task) {

I found the name of this confusingly close to `do_marking_step` by the task.  I can see how this could be called a "step" with preclean being another (new) step, but I think the confusion with the other function outweighs that and needs a different name here.  Maybe just `do_marking` or `do_all_marking` or something like that.

src/hotspot/share/gc/shared/referenceProcessor.cpp line 1073:

> 1071:   Ticks preclean_start = Ticks::now();
> 1072: 
> 1073:   ReferenceType ref_type_arr[4] = { REF_SOFT, REF_WEAK, REF_FINAL, REF_PHANTOM };

Instead of specifying the array size, just let it be computed from the initializer length.

src/hotspot/share/gc/shared/referenceProcessor.cpp line 1074:

> 1072: 
> 1073:   ReferenceType ref_type_arr[4] = { REF_SOFT, REF_WEAK, REF_FINAL, REF_PHANTOM };
> 1074:   size_t ref_count_arr[4] = {};

Instead of literal 4 here and below, use ARRAY_SIZE(ref_type_arr), or maybe give that value a name and replace all the literal 4s with that name.

src/hotspot/share/gc/shared/referenceProcessor.cpp line 1076:

> 1074:   size_t ref_count_arr[4] = {};
> 1075: 
> 1076:   if (discovery_is_mt()) {

The old form of these loops called `yield->should_return()` for each queue.  That's been lost in this rewrite, and I'm not sure that's correct.

src/hotspot/share/gc/shared/referenceProcessor.cpp line 1096:

> 1094:   }
> 1095: 
> 1096:   uint worker_id = WorkerThread::current()->id();

This logging is potentially far more verbose than previously, since there will now be a line for each concurrent marking thread, rather than one line for the previously single-threaded precleaning.  We have mechanisms for collecting and reporting timing and work units for parallel activities that should be used here.

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

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