RFR (M): 8201492: Properly implement non-contiguous generations for reference discovery

[Thomas Schatzl]

Hi Kim,

  thanks for your review.

On Mon, 2018-04-30 at 10:12 -0400, Kim Barrett wrote:
> > On Apr 29, 2018, at 2:45 PM, Thomas Schatzl <thomas.schatzl at oracle.
> > com> wrote:
> > Webrevs
> > http://cr.openjdk.java.net/~tschatzl/8201492/webrev.1 (full)
> > http://cr.openjdk.java.net/~tschatzl/8201492/webrev.0_to_1 (diff)
> > 
> > I pushed the changes through hs-tier1-3 again.
> > 
> > Note that the diff webrev is not very helpful in this case.
> > 
> > Thanks,
> >  Thomas
> 
> -------------------------------------------------------------------
> -----------
> The member name _span_discoverer, used by several of the collectors,
> might be confusing; it's not discovering spans.  I'm not sure what a
> better name might be though.  _span_based_discoverer?

I changed it to the suggested name.

> src/hotspot/share/gc/g1/g1FullCollector.hpp
>   47 class G1FullGCSubjectToDiscoveryClosure: public
> BoolObjectClosure {
>   48 public:
>   49   bool do_object_b(oop p) {
>   50     return (p != NULL);
>   51   }
>   52 };
> 
> I think no null check is needed here, and that p could instead be
> asserted to be non-NULL, because Full-GC is
> discovery_is_atomic.  This is similar to the young/mixed gc case
> (G!STWSubjectToDiscoveryClosure), and different from the cm case.
> That would better match the _always_subject_to_discovery name too.

Fixed.

> 
> -------------------------------------------------------------------
> ----------- 
> src/hotspot/share/gc/shared/referenceProcessor.hpp
>  489   ReferenceProcessorSpanMutator(ReferenceProcessor* rp,
>  490                                 MemRegion span):
>  491     _rp(rp), _discoverer(span) {
>  492     _old_discoverer = rp->is_subject_to_discovery_closure();
>  493     rp->set_is_subject_to_discovery_closure(&_discoverer);
>  494   }
> 
> Why is _old_discoverer initialized in the body rather than the
> initializer list.
> 
> Also, formatting makes it hard to spot where the initializer list
> ends and the body begins.

Done and added a newline.

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

Thanks,
  Thomas