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

[Per Liden]

On Tue, 24 Nov 2020 03:05:19 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.
>
> Kim Barrett has updated the pull request incrementally with two additional commits since the last revision:
> 
>  - new tests need ref in oldgen too
>  - remove obsolete comment about races with clear and enqueue

Looks good!

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

Marked as reviewed by pliden (Reviewer).

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