RFR (M): 8247819: G1: Process strong OopStorage entries in parallel

[Kim Barrett]

> On Jun 25, 2020, at 7:53 AM, Thomas Schatzl <thomas.schatzl at oracle.com> wrote:
> 
> Hi Stefan,
> 
>  thanks for your review.
> 
> On 25.06.20 13:28, Stefan Karlsson wrote:
>> The formatting looked weird. I'll try again:
>> Hi Thomas,
>> This isn't needed after we rewrote the OopStorageSetParState:
>> +// Needed by _oop_storage_set_strong_par_state as the definition is in the
>> +// .inline.hpp file.
>> +G1RootProcessor::~G1RootProcessor() {}
> 
> Removed.
> 
>> ---
>> This doesn't seem to be used:
>>  +
>>  + template <typename Closure>
>>  + static void strong_oops_do(Closure* cl); };
> 
> The method is still used by Parallel and Serial GC.
> 
>> ---
>> Just a suggestion to lower the noise:
>> + G1GCPhaseTimes::GCParPhases phase = G1GCPhaseTimes::GCParPhases(G1GCPhaseTimes::StrongOopStorageSetRoots + i);
>> could be changed to:
>> + G1GCPhaseTimes::GCParPhases phase(G1GCPhaseTimes::StrongOopStorageSetRoots + i);
> 
> This does not work (compile). G1GCPhaseTimes::GCParPhases is an enum, not a class.
> 
> New webrevs:
> http://cr.openjdk.java.net/~tschatzl/8247819/webrev.0_to_1/ (diff)
> http://cr.openjdk.java.net/~tschatzl/8247819/webrev.1/ (full)
> 
> Testing:
> recompilation
> 
> Thanks,
>  Thomas

Looks good.

One minor thing, for which I don’t need a new webrev if you take this suggestion:

------------------------------------------------------------------------------
src/hotspot/share/gc/g1/g1GCPhaseTimes.cpp
  69   int counter = 0;
  70   for (OopStorageSet::Iterator it = OopStorageSet::strong_iterator(); !it.is_end(); ++it, ++counter) {
...
  75     uint index = G1GCPhaseTimes::StrongOopStorageSetRoots + counter;

Rather than separate counter and index, maybe

  uint index = G1GCPhaseTimes::StrongOopStorageSetRoots;
  for (... ++index) {

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