RFR: 8188055: (ref) Add Reference.refersTo predicate

[Per Liden]

Hi Kim,

On 4/8/20 2:25 AM, Kim Barrett wrote:
> [Note review on both core-libs and hotspot-gc-dev lists; try not to lose
> either when replying.]
> 
> Please review a new function: java.lang.ref.Reference.refersTo.

Nice!

> 
> 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 a native method whose implementation is in the
> VM so it can use the Access API.  It is the intent that this function
> 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.

Do we have an enhancement filed for this? We should make it very clear 
that such an intrinsic is not allowed to safepoint between loading the 
referent and comparing it.

> 
> CR:
> https://bugs.openjdk.java.net/browse/JDK-8188055
> https://bugs.openjdk.java.net/browse/JDK-8241029 (CSR)
> 
> Webrev:
> https://cr.openjdk.java.net/~kbarrett/8188055/open.04/

The patch looks good to me, just two minor comments:


java/lang/ref/Reference.java
----------------------------

  349      * @param   obj is the object to compare with the referenced 
object, or
  350      *          {@code null}.

May I suggest "referenced object" -> "referent" to keep the nomenclature 
intact.


gc/TestReferenceRefersTo.java
-----------------------------

  208             long timeout = 1000;
  209             while (true) {
  210                 Reference<? extends TestObject> ref = 
queue.remove(timeout);
  211                 if (ref == null) {
  212                     break;
  213                 } else if (ref == testPhantom1) {
  214                     testPhantom1 = null;
  215                 } else if (ref == testWeak2) {
  216                     testWeak2 = null;
  217                 } else if (ref == testWeak3) {
  218                     testWeak3 = null;
  219                 } else if (ref == testWeak4) {
  220                     testWeak4 = null;
  221                 } else {
  222                     fail("unexpected reference in queue");
  223                 }
  224             }

That timeout look unnecessarily short, and I imagine it could cause 
intermittent failures on really slow machines. I would suggest we make 
that more like 60 seconds, alternatively skip the timeout all together 
and let the whole test timeout if the expected references don't appear 
in the queue.

cheers,
Per

> 
> Testing:
> mach5 tier1
> 
> Locally (linux-x64) verified the new test passes with various garbage
> collectors.
>