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

[Per Liden]

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 58:

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

This part, and the use of obj3, below seems odd and unnecessary. If the Reference doesn't have the expected value
there's no special reason to think it has obj3 as value (as opposed to something else). I don't think this check will
help debugging if this test fails.

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

> 40:             this.value = value;
> 41:         }
> 42:     }

Introducing this class seems unnecessary, since its `value` is never used. So all instances of `TestObject` could just
be `Object`.

test/hotspot/jtreg/gc/TestReferenceRefersTo.java line 186:

> 184:
> 185:             expectNotValue(testWeak2, testObject3, "testWeak2");
> 186:             expectValue(testWeak3, testObject3, "testWeak3");

It looks to me like the `expectNotCleared()` and `expectNotValue()` methods can be removed. All expect-tests can be
done with just `expectCleared()` and `expectValue()`.

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

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