RFR: 8188055: (ref) Add Reference::refersTo predicate [v2]

[Mandy Chung]

On Thu, 8 Oct 2020 08:15:07 GMT, Kim Barrett <kbarrett at openjdk.org> wrote:

>> Finally returning to this review that was started in April 2020.  I've
>> recast it as a github PR.  I think the security concern raised by Gil
>> has been adequately answered.
>> https://mail.openjdk.java.net/pipermail/hotspot-gc-dev/2020-April/029203.html
>> https://mail.openjdk.java.net/pipermail/hotspot-gc-dev/2020-July/030401.html
>> https://mail.openjdk.java.net/pipermail/hotspot-gc-dev/2020-August/030677.html
>> https://mail.openjdk.java.net/pipermail/hotspot-gc-dev/2020-September/030793.html
>> 
>> Please review a new function: java.lang.ref.Reference.refersTo.
>> 
>> This function is needed to test the referent of a Reference object without
>> artificially extending the lifetime of the referent object, as may happen
>> when calling Reference.get.  Some garbage collectors require extending the
>> lifetime of a weak referent when accessed, in order to maintain collector
>> invariants.  Lifetime extension may occur with any collector when the
>> Reference is a SoftReference, as calling get indicates recent access.  This
>> new function also allows testing the referent of a PhantomReference, which
>> can't be accessed by calling get.
>> 
>> The new function uses native methods whose implementations are in the VM so
>> they can use the Access API.  It is the intent that these methods will be
>> intrinsified by optimizing compilers like C2 or graal, but that hasn't been
>> implemented yet.  Bear that in mind before rushing off to change existing
>> uses of Reference.get.
>> 
>> There are two native methods involved, one in Reference and an override in
>> PhantomReference, both package private in java.lang.ref. The reason for this
>> split is to simplify the intrinsification. This is a change from the version
>> from April 2020; that version had a single native method in Reference,
>> implemented using the ON_UNKNOWN_OOP_REF Access reference strength category.
>> However, adding support for that category in the compilers adds significant
>> implementation effort and complexity. Splitting avoids that complexity.
>> 
>> Testing:
>> mach5 tier1
>> Locally (linux-x64) verified the new test passes with various garbage collectors.
>
> Kim Barrett has updated the pull request incrementally with five additional commits since the last revision:
> 
>  - improve refersTo0 descriptions
>  - basic functional test
>  - change referent access
>  - expand test
>  - remove CMS comment

test/jdk/java/lang/ref/ReferenceRefersTo.java line 26:

> 24: /*
> 25:  * @test
> 26:  * @summary Basic functional test of Reference.refersTo.

Thanks for adding this basic test.  Can you add `@bug 8188055`

test/jdk/java/lang/ref/ReferenceRefersTo.java line 49:

> 47:
> 48:     private static final <T extends Reference>
> 49:     void test(T ref,

Nit: I prefer this is merged with line 48 and the line is not too awfully long.

test/jdk/java/lang/ref/ReferenceRefersTo.java line 57:

> 55:         } else if (!ref.refersTo(expectedValue)) {
> 56:             fail(kind + " doesn't refer to expected value");
> 57:         } else if (ref.refersTo(unexpectedValue)) {

Nit: It seems easier to read if these are 3 separate checks, i.e. dropping `else`

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

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