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

[Thomas Schatzl]

Hi,

On Tue, 2018-06-05 at 17:41 -0400, Kim Barrett wrote:
> > On Jun 5, 2018, at 8:27 AM, Thomas Schatzl <thomas.schatzl at oracle.c
> > om> 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.

Okay. The reason I used "PhaseX" was because any identifiers I could
come up with were very long, particularly for phase 3.

I changed it to your naming (I allowed myselves to drop the "object",
please tell me if you think it is required), but I am hoping that
somebody has some more succinct suggestions.

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

Okay, added a "Total" line.

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

Since this would make the output inconsist with the evacuation phase
where thread numbers are printed always I left that out.

Also these are the numbers of threads contributing work, not the number
of work items the task has been started with. This is only the same
because at the moment the time tracker scoped objects are encapsulating
 the whole work methods regardless of whether there is work to do (i.e.
their list is empty), so the always get a "0.0" entry.

That information would be interesting, but I would prefer to do that in
an extra CR.

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

Fixed.

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

:) Fixed.

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

Fixed.

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

Fixed.

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

Fixed, since this is about trace messages.

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

Fixed.

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

I thought marks_oops_alive is an unnecessary quirk of the collectors
that found its way into the reference processing interface; its purpose
is to indicate whether the gc should perform stealing at the end of a
phase, as the complete closures drain the stacks completely by
themselves anyway.

Stealing or not should be part of the complete closure (i.e. the gc,
and attempted by default by the gc) imho (it "completes" the phase),
like G1 does.

I fixed it though.

New webrevs:
http://cr.openjdk.java.net/~tschatzl/8202845/webrev.0_to_1 (diff)
http://cr.openjdk.java.net/~tschatzl/8202845/webrev.1 (full)
Testing:
hs-tier1-5,jdk-tier-1-3 (with +/-ParallelRefProcEnabled)

Thanks for your quick review,
  Thomas