RFR: 8256517: (ref) Reference.clear during reference processing may lose notification

[Per Liden]

On Mon, 23 Nov 2020 01:43:39 GMT, Kim Barrett <kbarrett at openjdk.org> wrote:

> Please review this change to Reference.clear() to address several issues.
> 
> (JDK-8240696) For GCs using a SATB barrier, simply assigning the referent
> field to null may extend the lifetime of the referent value.
> 
> (JDK-8240696) For GCs with concurrent reference processing, clearing the
> referent field during reference processing may discard the expected
> notification.
> 
> Both of these are addressed by introducing a private native helper function
> for clearing the referent, rather than using an ordinary in-Java field
> assignment. Tests have been added for both of these issues. This required
> adding a new breakpoint in reference processing for ZGC.
> 
> Of course, finalization adds some complexity to the problem.  We deal with
> that by having FinalReference override clear.  The implementation is
> provided by a new package-private method in Reference.  (There are a number
> of alternatives, all of them clumsy; finalization is annoying that way.)
> 
> While dealing with FinalReference clearing it was noted that the recent
> JDK-8256106 and JDK-8256370 have some problems. FinalizerHistogram was not
> updated to call the new Reference.getInactive(), instead still calling get()
> on FinalReferences, with the JDK-8256106 problems. Fixing that showed the
> assertion for inactive FinalReference added by JDK-8256370 used the wrong
> test.
> 
> Rather than tracking down and changing all get() and clear() calls on final
> references and changing them to use getInactive and a new similar clear
> function, I've changed FinalReference to override get and clear, which call
> the helper functions in Reference. I've also renamed getInactive to be more
> explanatory and less convenient to call directly, and similarly named the
> helper for clear. This means that get/clear should never be called on an
> active FinalReference. That's already never done, and would have problems
> if it were.
> 
> Testing:
> mach5 tier1-6
> Local (linux-x64) tier1 using Shenandoah.
> New TestReferenceClearDuringMarking fails for G1 without these changes.
> New TestReferenceClearDuringReferenceProcessing fails for ZGC without these changes.

Looks good. Just want to request that you also remove the following comment in zReferenceProcessor.cpp, as it's no longer true.

--- a/src/hotspot/share/gc/z/zReferenceProcessor.cpp
+++ b/src/hotspot/share/gc/z/zReferenceProcessor.cpp
@@ -184,12 +184,6 @@ bool ZReferenceProcessor::should_discover(oop reference, ReferenceType type) con
 }
 
 bool ZReferenceProcessor::should_drop(oop reference, ReferenceType type) const {
-  // This check is racing with a call to Reference.clear() from the application.
-  // If the application clears the reference after this check it will still end
-  // up on the pending list, and there's nothing we can do about that without
-  // changing the Reference.clear() API. This check is also racing with a call
-  // to Reference.enqueue() from the application, which is unproblematic, since
-  // the application wants the reference to be enqueued anyway.
   const oop referent = reference_referent(reference);
   if (referent == NULL) {
     // Reference has been cleared, by a call to Reference.enqueue()

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

Changes requested by pliden (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/1376