RFR: 8203028: Simplify reference processing in light of JDK-8175797

[Kim Barrett]

> On May 14, 2018, at 8:45 AM, Thomas Schatzl <thomas.schatzl at oracle.com> wrote:
> 
> Hi Kim,
> 
>  some initial comments…

Thanks for looking at this.

> On Sun, 2018-05-13 at 21:32 -0400, Kim Barrett wrote:
>> […]
>> CR:
>> https://bugs.openjdk.java.net/browse/JDK-8203028
>> 
>> Webrev:
>> http://cr.openjdk.java.net/~kbarrett/8203028/open.00/
>> 
> 
> - please update to latest jdk/jdk, there are some merge conflicts with
> the precleaning for g1 change.

OK, webrev open.01 is based on repo with the precleaning change (which
hadn't been pushed when I sent the RFR).

> - please remove the unused complete_gc parameter from
> ReferenceProcessor::process_phase2.

I intentionally left it alone.  Removing it completely starts getting
messy when one starts walking up the callers and hits the parallel
task layer (either drop the argument there instead, or have two
different work functions with consequent fannout).  And I expect this
interface is going to be completely changed by JDK-8202845, so it
didn't seem worth doing much here.

> - in ReferenceProcessor::process_phase2, the comment about the
> iter.referent() == NULL path only being possible when discovery is not
> atomic should be reinforced with an assert.

There is already an assert in the immediately preceeding load_ptrs
that either the referent is non-null or the argument
(!discovery_is_atomic()) is true.  I've augmented the comment to refer
to the preceeding load_ptrs.

> - Reference.java, comments:
> 
> 1) "Active: Subject to special treatment by the garbage
> collector.  Some  time after the collector detects that the
> reachability of the referent has changed to the appropriate state, the
> collector "notifies" the reference, changing the state to either
> "pending" or "inactive".
> 
> referent != null; discovered = null, or in GC discovered list."
> 
> Could this be more specific than "appropriate state" - wouldn't this be
> always "unreachable”?

"Unreachable" is not the right term here; the appropriate states are
described in package-info.java (e.g. softly/weakly/phantom-reachable)
and JLS 12.6 (finalizer reachable).

> Also, referent may be NULL in case of mutators clear()'ing (or
> enqeue()'ing) the Reference in case of concurrent reference processing.
> I would prefer to make this more accurate than misleading.

Not just concurrent reference processing, also for concurrent
discovery (which is what G1 and CMS do, not concurrent reference
processing). Either of those operations makes the object not Active;
see the state transitions table later in that comment.

> 2) "The collector only needs to examine the referent field and the
> discovered field to determine whether a normal Reference object needs
> special treatment.  If the referent is non-null and not known to be
> live, then it may need to be discovered for possible later
> notification.  But if the discovered field is non-null, then either (1)
> it has already been discovered, or (2) it is in the pending list."
> 
> What is a "normal" Reference object (and what is a not-normal one?)

Normal != FinalReference.  The latter is described in the next
paragraph as being different.  But I've tweaked things a bit to be
more explicit and hopefully more clear.

> 3) "FinalReference differs from other references, because a
> FinalReference is not cleared when notified.  The referent being null
> or not cannot be used to distinguish between the active state and
> pending or inactive states.  However, FinalReferences do not support
> enqueue().  Instead, the next field of a FinalReference object is set
> to the object when it is added to the pending list, and the use of this
> as the value of next in the enqueued and dequeued states maintains the
> non-active state.  An additional check that the next field is null is
> required to determine that a FinalReference object is active."
> 
> - instead of "is set to the object" "is set to "this"" would be better
> to avoid the question what "set to the object" means.

Done.

> - " of a FinalReference object is set to the object when it is added to
> the pending list, and the use of this as the value of next in the
> enqueued and dequeued states maintains the non-active state."
> 
> Please split the sentence between "list" and "and". Also I would prefer
> if "this" were quoted to make sure the value "this" is meant and not
> referring to something else (I know the entire file does not do that,
> so ignore if you think this is a bad idea)

Done.

New webrev, rebased to include JDK-8201491:
http://cr.openjdk.java.net/~kbarrett/8203028/open.01/

New webrevs with above changes:
full: http://cr.openjdk.java.net/~kbarrett/8203028/open.02/
incr: http://cr.openjdk.java.net/~kbarrett/8203028/open.02.inc/