RFR (XL): 8202845: Refactor reference processing for improved parallelism

[Kim Barrett]

> On Jun 12, 2018, at 2:19 PM, Thomas Schatzl <thomas.schatzl at oracle.com> wrote:
> On Tue, 2018-06-12 at 12:20 -0400, Kim Barrett wrote:
>> src/hotspot/share/gc/shared/referenceProcessor.cpp
>> 571     complete_gc.do_void();
>> 
>> I think this is unavoidable (at least for now), but I'm wondering if
>> this is a performance issue for other than G1-young collections.
>> 
>> In the phases where marks_oops_alive is false (like this one), for
>> the other collectors (and G1 concurrent and full GC, I think), there
>> won't be anything to steal.
> 
> Why? Any imbalance in how fast the work will be processed will need
> stealing to keep the threads doing useful work.
> Particularly traversing and keeping alive followers is highly dependent
> on the object graph referenced by the referent.

[Thomas and I discussed this offline.  This is for anyone else
who might be following along.]

Because the threads aren't generating any work for others to steal,
for collections other than G1-young/mixed.  This is distinct from any
allocation of references to be processed to threads.  This is about
dealing with referents to which the keep_alive closure was applied.
In these phases, for some collections the keep_alive is just an
expensive nop, finding that the referent is already marked live (which
we knew because the is_alive closure already told us that).  For
others, it needs to forward the referent field to the already copied
referent; no additional work (e.g. scanning the referent) is
needed. The only current keep_alive closure that creates any work for
the complete_gc closure in these cases is G1CopyingKeepAliveClosure.
The old process_phase2 assumed the complete_gc closure would never
have any work to do, so didn't bother calling it.

>> Starting the stealing process (and its associated termination
>> protocol) is a waste of time in those cases.
> 
> Starting the stealing process is basically free - at most what these do
> is looking through the work queues whether they are empty - this is a
> normal load of a few variables.
> 
> Shenandoah's task terminator is very good at detecting these cases with
> very little or no work to be done and not waking up anyone (i.e.
> terminating very quickly).
> 
> I have been running almost all test runs for all patches sent in the
> last month or so with it with no problem, so it will likely be a change
> for the first few weeks for jdk12.

Oh, that sounds great! Looking forward to it.

>> ParallelScavange and ParallelCompaction both avoid creating the
>> stealing tasks if marks_oops_alive is false.
> 
> This is actually the only case where the flag makes some sense because
> parallel gc needs to set the tasks beforehand.
> 
> But we may want to look into making parallel gc use workgangs too,
> because the current implementation of the task queue in parallel is
> very problematic as it is a single point of contention every time a
> task ends, and it causes us to use lots of boiler plate code to support
> both workgang and what parallel gc uses.
> 
>> CMS avoids calling do_work_stealing when marks_oop_live is
>> false.  ParNew doesn't seem to look at that bit though.
>> 
>> G1 concurrent and full GCs also don't seem to look at that bit, and
>> so I think will be negatively affected.  Maybe this is a job for
>> another RFE.
> 
> Yes, we will probably need to spend more time in this area :)

Agreed.

> 
>> Some comment revision might be called for though.  It would be nice
>> to not have to re-discover (no pun intended) all this again.
>> 
>> -------------------------------------------------------------------
>> -----------
>> src/hotspot/share/gc/shared/referenceProcessor.cpp 
>> 457   // Close the reachable set
>> 458   complete_gc->do_void();
>> 
>> Same issues here for PhantomReferences as earlier for
>> Soft/Weak/FinalReferences.  And I think the same solution; pay
>> attention to the marks_oops_alive bit.
> 
> - fix the task terminator (we need that anyway)
> - let the collector decide whether it has work
> - fix the parallel gc work gang
> 
> :)

Agreed.

> http://cr.openjdk.java.net/~tschatzl/8202845/webrev.1_to_2 (diff)
> http://cr.openjdk.java.net/~tschatzl/8202845/webrev.2 (full)

Looks good.