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

[Albert Mingkun Yang]

On Sat, 13 Nov 2021 04:49:25 GMT, Kim Barrett <kbarrett at openjdk.org> wrote:

>> 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.
>
> 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.

Renamed to `do_marking`.

> 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.

Fixed.

> 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.

Fixed.

> 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.

It's still correct because `preclean_discovered_reflist` checkes for if-aborted while iterating the list. I could have written sth like the following if you think prompt abort is important.


  bool is_aborted = preclean_discovered_reflist(...);
  if (is_aborted) {
    return;
  }

> 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.

The new logs are on `trace` level, just FYI. How about I address all logging related issues (from Thomas and Stefan as well) in a follow-up PR?

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

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