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

[Kim Barrett]

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

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

------------------------------------------------------------------------------ 
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);

That is, the one-arg form of is_obj_ill performs the null check, the
is_in_reserved assert (though using is_in_g1_reserved), and the heap
region lookup.

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

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