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

[Thomas Schatzl]

Hi,

On Fri, 2018-04-27 at 21:23 -0400, Kim Barrett wrote:
> > On Apr 18, 2018, at 2:52 AM, Thomas Schatzl <thomas.schatzl at oracle.
> > com> wrote:
> > 
> > CR:
> > https://bugs.openjdk.java.net/browse/JDK-8201492
> > Webrev:
> > http://cr.openjdk.java.net/~tschatzl/8201492/webrev/
> > Testing:
> > hs-tier1-3, performance verification, lots of Kitchensink reference
> > stress testing runs
> > 
> 
> StefanJ said:
>   The only thing I can comment on is the introduction of
>   SpanReferenceProcessor. I think it would be nicer to just have a
>   single ReferenceProcessor that always take a closure for deciding
> if
>   discovery should be done, ie. let the other GCs explicitly create
>   their SpanBasedDiscoverer and pass it in.
> 
> I had the same reaction.  It does provide [set_]span(); is that
> enough
> to make it worthwhile?  Maybe?
> 
> If that were changed, it may impact other comments below.

Done.

> 
> -------------------------------------------------------------------
> ----------- 
> src/hotspot/share/gc/shared/referenceProcessor.cpp 
>  135 SpanReferenceProcessor::SpanReferenceProcessor(MemRegion span,
> ...
>  142   ReferenceProcessor(&_span_based_discoverer,
> ...
>  149   _span_based_discoverer(span) {
> 
> Passing a pointer to a member before the member has been constructed
> can lead to problems, though I didn't find any issues here.  However,
> I've seen compiler warnings at some warning levels for something like
> this.  (Obviously not with the levels we're testing with.)
> 
> Obviously, this isn't a problem if there isn't a
> SpanReferenceProcessor.
> 
> -------------------------------------------------------------------
> ----------- 
> src/hotspot/share/gc/shared/referenceProcessor.cpp 
>  977 template <class T>
>  978 bool ReferenceProcessor::is_subject_to_discovery(T const obj)
> const {
>  979   return _is_subject_to_discovery->do_object_b(obj);
>  980 }
> 
> I don't understand why this is a template.  T *must* be oop (or at
> least convertible to oop), since do_object_b takes an oop argument.

Fixed.

> 
> -------------------------------------------------------------------
> ----------- 
> src/hotspot/share/gc/g1/g1ConcurrentMark.inline.hpp
>   41 inline bool G1CMIsAliveClosure::do_object_b(oop obj) {
>   42   if (obj == NULL) {
>   43     return false;
>   44   }
>   45   assert(_g1h->is_in_reserved(obj), "Asked for liveness of oop "
> PTR_FORMAT " outside of reserved heap.", p2i(obj));
>   46   // Young regions have nTAMS == bottom(), i.e. all objects
> there are implicitly live,
>   47   // so we do not need to explicitly check for region type.
>   48   bool result = !_g1h->is_obj_ill(obj, _g1h-
> >heap_region_containing(obj));
> 
> I think the code in lines 42-48 is equivalent to
> 
>   bool result = !_g1h->is_obj_ill(obj);
> 

It is. However the assert error message would not be that descriptive
(imho). I changed it to the suggested 

  42   return !_g1h->is_obj_ill(obj);

though.

> -------------------------------------------------------------------
> ----------- 
> src/hotspot/share/gc/g1/g1ConcurrentMark.inline.hpp
>   55 inline bool G1CMSubjectToDiscoveryClosure::do_object_b(oop obj)
> {
>   56   if (obj == NULL) {
>   57     return false;
>   58   }
> 
> I haven't checked carefully, but it seems wrong to check for
> discoverability of NULL; I would have expected NULL to already have
> been filtered out before getting to such a check.  In which case the
> check here ought to be an assert != NULL.
> 
> Oh, but when the policy is ReferentBasedDiscovery we apply this
> closure to the referent, and for a concurrent collector the referent
> could have been cleared between the time the referent was determined
> to be (potentially) not alive and when this check is made.  That's
> pretty subtle, and seems worth a comment.

Added.

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