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

[Kim Barrett]

> On Jun 5, 2018, at 8:27 AM, Thomas Schatzl <thomas.schatzl at oracle.com> wrote:
> CR:
> https://bugs.openjdk.java.net/browse/JDK-8202845
> Webrev:
> http://cr.openjdk.java.net/~tschatzl/8202845/webrev/
> Testing:
> hs-tier1-5,jdk-tier1-3
> 
> Thanks,
>  Thomas


------------------------------------------------------------------------------ 
Logging:

I'm not sure using Phase{1,2,3,4} is the right approach.  I think it
might be better to use more informative descriptions here, e.g.
something like

Phase1 => Reconsider SoftReferences
Phase2 => Notify Soft/WeakReferences
Phase3 => Notify and keep alive finalizable objects
Phase4 => Notify PhantomReferences

That gives more information to the reader.  It also doesn't suffer
from renumbering when we make further planned changes, like moving
Phase1 elsewhere, and (someday) elimination of finalization.

------------------------------------------------------------------------------ 
Logging:

Phase2 timing information is broken down by reference type, which may
be useful.  But timing information about the phase as a whole only
contains the total time, without any per-thread breakdown for the
phase as a whole.  That information seems equally useful.

Also, the number of workers is being repeated, even though it doesn't
change over the phase.

------------------------------------------------------------------------------
src/hotspot/share/gc/shared/referenceProcessor.cpp

Throughout, need_balance_queues is being applied to _discovered_refs,
which is incorrect.  (That's effectively a synonym for
_discoveredSoftRefs.) It needs to be applied to each specific
refs_lists[].

------------------------------------------------------------------------------ 
src/hotspot/share/gc/shared/referenceProcessor.cpp

process_soft_weak_final_drop_live_null seems like an excessively long
name, and isn't really more accurate or informative than something
like process_soft_weak_final_refs.

------------------------------------------------------------------------------ 
src/hotspot/share/gc/shared/referenceProcessor.hpp

Please ensure blank lines between the various process_xxx declarations
and the comments that follow them.

Circa lines 285 and 293.

------------------------------------------------------------------------------ 
src/hotspot/share/gc/shared/referenceProcessor.cpp
 425   complete_gc->do_void();
 426   iter.complete_enqueue();
...
 456   complete_gc->do_void();
 457   iter.complete_enqueue();

These are changing to the ordering of those two operations; they were
done in the other order in the old process_phase3.  I'm not sure it
matters, but I wasn't expecting such a change.

------------------------------------------------------------------------------ 
src/hotspot/share/gc/shared/referenceProcessor.cpp
 327 size_t ReferenceProcessor::process_soft_ref_reevaluate_work(DiscoveredList&    refs_list,
...
 340       log_develop_trace(gc, ref)("Dropping reference (" INTPTR_FORMAT ": %s"  ") by policy",
 341                                  p2i(iter.obj()), iter.obj()->klass()->internal_name());

[pre-existing]

Klass::internal_name() may perform resource allocation, but there's no
obvious ResourceMark near here, unlike in
process_final_keep_alive_work and process_phantom_refs.

Same issue in process_soft_weak_final_drop_live_null_work.

I wonder if some of the ResourceMarks should be moved into
log_(dropped|enqueued)_ref, rather than being outside a loop with an
unknown number of resource allocations.  And similarly in
process_soft_ref_reevaluate_work. 

------------------------------------------------------------------------------ 
src/hotspot/share/gc/shared/referenceProcessor.cpp
 792       removed += process_soft_ref_reevaluate_work(_discoveredSoftRefs[i], _current_soft_ref_policy,
 793                                 is_alive, keep_alive, complete_gc);

Abnormal indentation.

------------------------------------------------------------------------------ 
src/hotspot/share/gc/shared/referenceProcessor.hpp

Changed to remove marks_oops_alive from ProcessTask.

I'm not sure why this change was made.  It would seem to make some
collectors terminate Phase2 more slowly than necessary, since every
thread will end with an unnecessary stealing segment that should never
find anything to do, but needs to rendezvous with all the other
threads.

Note that JDK-8203028 missed changing the construction of the
RefProcPhase2Task; it should have changed the marks_oops_alive
argument unconditionally false.

This is another piece of the mostly implicit / lightly documented
contract for the is_alive closure that G1 is violating.

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