Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8188055: (ref) Add Reference::refersTo predicate #498

Closed
wants to merge 13 commits into from

Conversation

kimbarrett
Copy link

@kimbarrett kimbarrett commented Oct 4, 2020

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.

/csr needed
/label hotspot-gc, core-libs


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

Reviewers

Download

$ git fetch https://git.openjdk.java.net/jdk pull/498/head:pull/498
$ git checkout pull/498

@kimbarrett kimbarrett changed the title new function 8188055: (ref) Add Reference::refersTo predicatenew function Oct 4, 2020
@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Oct 4, 2020

👋 Welcome back kbarrett! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@kimbarrett kimbarrett changed the title 8188055: (ref) Add Reference::refersTo predicatenew function 8188055: (ref) Add Reference::refersTo predicate Oct 4, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Oct 4, 2020

@kimbarrett usage: /csr [needed|unneeded], requires that the issue the pull request refers to links to an approved CSR request.

@openjdk openjdk bot added the hotspot-gc label Oct 4, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Oct 4, 2020

@kimbarrett
The hotspot-gc label was successfully added.

The core-libs label was successfully added.

@openjdk openjdk bot added the core-libs label Oct 4, 2020
@kimbarrett kimbarrett marked this pull request as ready for review Oct 5, 2020
@openjdk openjdk bot added the rfr label Oct 5, 2020
@mlbridge
Copy link

@mlbridge mlbridge bot commented Oct 5, 2020

Webrevs


// For some collectors, calling get() will keep testObject4 alive.
// For example, SATB collectors or ZGC, but not collectors using
// incremental update barriers like CMS.
Copy link
Contributor

@pliden pliden Oct 5, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove obsolete CMS comment?

private static PhantomReference<TestObject> testPhantom1 = null;

private static WeakReference<TestObject> testWeak2 = null;
private static WeakReference<TestObject> testWeak3 = null;
Copy link
Contributor

@pliden pliden Oct 5, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like test*2 and test*3 test the same thing in both tests, so test*3 could be removed, and test*4 could be renamed test*3.

template<DecoratorSet on_ref>
static bool referenceRefersTo(jobject ref, jobject o) {
const int offset = java_lang_ref_Reference::referent_offset();
oop ref_oop = JNIHandles::resolve_non_null(ref);
oop referent = HeapAccess<on_ref | AS_NO_KEEPALIVE>::oop_load_at(ref_oop, offset);
return referent == JNIHandles::resolve(o);
}
Copy link
Contributor

@pliden pliden Oct 5, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about moving the inner logic to java_lang_ref_{Reference,PhantomReference}::refers_to() functions?

JVM_ENTRY(jboolean, JVM_ReferenceRefersTo(JNIEnv* env, jobject ref, jobject o))
JVMWrapper("JVM_ReferenceRefersTo");
return referenceRefersTo<ON_WEAK_OOP_REF>(ref, o);
JVM_END
Copy link
Contributor

@pliden pliden Oct 5, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

... and let this be something like:

JVM_ENTRY(jboolean, JVM_ReferenceRefersTo(JNIEnv* env, jobject ref, jobject o))
  JVMWrapper("JVM_ReferenceRefersTo");
  oop ref_oop = JNIHandles::resolve_non_null(ref);
  oop cmp_oop = JNIHandles::resolve(o);
  return java_lang_ref_Reference::refers_to(ref_oop, cmp_oop);
JVM_END

Copy link
Member

@mlchung mlchung Oct 6, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like Per's suggestion that makes it explicitly connect to Reference::refers_to.

JVM_ENTRY(jboolean, JVM_PhantomReferenceRefersTo(JNIEnv* env, jobject ref, jobject o))
JVMWrapper("JVM_PhantomReferenceRefersTo");
return referenceRefersTo<ON_PHANTOM_OOP_REF>(ref, o);
JVM_END
Copy link
Contributor

@pliden pliden Oct 5, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

... and let this be something like:

JVM_ENTRY(jboolean, JVM_PhantomReferenceRefersTo(JNIEnv* env, jobject ref, jobject o))
  JVMWrapper("JVM_PhantomReferenceRefersTo");
  oop ref_oop = JNIHandles::resolve_non_null(ref);
  oop cmp_oop = JNIHandles::resolve(o);
  return java_lang_ref_PhantomReference::refers_to(ref_oop, cmp_oop);
JVM_END

Copy link
Member

@mlchung mlchung left a comment

Can you add a unit test in test/jdk/java/lang/ref that serves as a basic API test for this new refersTo API without depending the WhiteBox API? test/hotspot/jtreg/gc` test serves as a more white-boxed type and comprehensive test.

JVM_ENTRY(jboolean, JVM_ReferenceRefersTo(JNIEnv* env, jobject ref, jobject o))
JVMWrapper("JVM_ReferenceRefersTo");
return referenceRefersTo<ON_WEAK_OOP_REF>(ref, o);
JVM_END
Copy link
Member

@mlchung mlchung Oct 6, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like Per's suggestion that makes it explicitly connect to Reference::refers_to.

}

/* Type-erased implementation of refersTo(). */
native boolean refersTo0(Object o);
Copy link
Member

@mlchung mlchung Oct 6, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any reason why it can't use the parameter type like refersTo0(T o)? Same for PhantomReference::refersTo0(T o)

Copy link
Member

@dfuch dfuch Oct 6, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But should refersTo takes a T? I mean - if I have a Reference<?> ref, the compiler will not let me query ref.refersTo(o); is this what we want?

Copy link
Member

@mlchung mlchung Oct 12, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dfuch I would expect the users of refersTo should know the type of the referent to be compared, i.e. it should know the parameter type of a Reference object. Do you have any specific use case in mind that you have a Reference<?> ref but wants to compare a referent of unknown type?

Copy link
Member

@dfuch dfuch Oct 13, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mlchung I have often used a Reference<?> in tests - but my main usage there would be to call ref.refersTo(null) which works in all cases.
My main concern here however is that using T in refersTo seems to go against the advertised usage of the method - I mean - if I have an object obj of type unknown, I can always do obj == ref.get() whatever the parameter type of ref is. But I won't be able to call ref.refersTo(obj) unless I use raw types and suppress warnings. So I just wanted to check that this was intentional.

Copy link
Member

@mlchung mlchung Oct 14, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the common cases, the application should know the type of the referent and using T in refersTo will benefit from the compiler type checking. For the unknown type case, cast to Reference<Object> is not ideal but reasonable? something like this:

Reference<Object> r = (Reference<Object>) ref;
r.refersTo(obj);

Copy link
Author

@kimbarrett kimbarrett Oct 24, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a consensus on this issue? It's not clear whether Daniel and Peter have agreed with Mandy's responses or have just not yet responded with further discussion.

Copy link
Contributor

@plevart plevart Oct 24, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mlchung I don't have many known use cases, but how about WeakHashMap.containsKey(Object key) for example? Currently WeakHashMap.Entry<K, V> extends WeakReference<Object> but it would be more type safe if it extended WeakReference<K>. In that case an entry.refersTo(key) would not compile...
What I'm trying to say is that even if Reference instances are not "leaked", you can get an untyped object reference from outside and you may want to identity-compare it with the Reference's referent.

Copy link
Member

@stuart-marks stuart-marks Oct 28, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some thoughts regarding the parameter type of refersTo. Summary: I think refersTo(T) is fine and that we don't want to change it to refersTo(Object).

I don't think we have a migration issue similar to generifying collections, where there was a possibility of changing contains(Object) to contains(E). If that had been done, it would have been a source compatibility issue, because changing the signature of the method potentially affects existing code that calls the method. That doesn't apply here because we're adding a new method.

The question now falls to whether it's preferable to have more convenience with refersTo(Object) or more type-safety with refersTo(T). With the generic collections issue, the migration issue probably drove the decision to keep contains(Object), but this has resulted in a continual set of complaints about the lack of an error when code passes an instance of the "wrong" type. I think that kind of error is likely to occur with refersTo. Since we don't have a source compatibility issue here, we can choose the safer API and avoid this kind of problem entirely.

The safer API does raise the possibility of having to add inconvenient unchecked casts and local variables in certain places, but I think Mandy's comment about the code already having a reference of the "right" type is correct. Her prototype webrev linked above shows that having to add unchecked casts is fairly infrequent.

Copy link
Contributor

@plevart plevart Oct 28, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some thoughts regarding the parameter type of refersTo. Summary: I think refersTo(T) is fine and that we don't want to change it to refersTo(Object).

I agree that we don't have a migration problem here that collections had. So let it be refersTo(T) then.

Copy link
Member

@dfuch dfuch Oct 28, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree as well.

* java.lang.ref.PhantomReference
*/
JNIEXPORT jboolean JNICALL
JVM_PhantomReferenceRefersTo(JNIEnv *env, jobject ref, jobject o);
Copy link
Member

@mlchung mlchung Oct 6, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of a separate PhantomReference::refersTo0 native method (and new PhantomReference.c), an alternative approach could be to detect if ref is an instance of PhantomReference or other Reference and get the referent accordingly.

/* Type-erased implementation of refersTo(). */
@Override
native final boolean refersTo0(Object o);

Copy link
Contributor

@RogerRiggs RogerRiggs Oct 6, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How does the existence of this method help future intrinsification?
If it was called somewhere it would be clearer.
Perhaps a comment now or later would help explain its function.

@mlbridge
Copy link

@mlbridge mlbridge bot commented Oct 8, 2020

Mailing list message from Kim Barrett on hotspot-gc-dev:

On Oct 5, 2020, at 7:04 AM, Per Liden <pliden at openjdk.java.net> wrote:
test/hotspot/jtreg/gc/TestReferenceRefersTo.java line 179:

177: // For some collectors, calling get() will keep testObject4 alive.
178: // For example, SATB collectors or ZGC, but not collectors using
179: // incremental update barriers like CMS.

Remove obsolete CMS comment?

I'll just delete lines 178-9.

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

63:
64: private static WeakReference<TestObject> testWeak2 = null;
65: private static WeakReference<TestObject> testWeak3 = null;

It looks like test*2 and test*3 test the same thing in both tests, so test*3 could be removed, and test*4 could be
renamed test*3.

The test seems to be missing some stuff. This looks like the test from the
webrev back in April too. I looked through some backups and never published
webrevs and found some other versions, but nothing that looks like the final
version I remember. Oh well. So I'm adding some additional checks that make
the 2 & 3 cases differ as intended.

src/hotspot/share/prims/jvm.cpp line 3446:

3444: oop referent = HeapAccess<on_ref | AS_NO_KEEPALIVE>::oop_load_at(ref_oop, offset);
3445: return referent == JNIHandles::resolve(o);
3446: }

How about moving the inner logic to java_lang_ref_{Reference,PhantomReference}::refers_to() functions?

I think refersTo functionality doesn't really belong in
java_lang_ref_Reference. I would rather keep the refersTo implementation in
jvm.cpp, but change the implementation from bare Access to using something
like java_lang_ref_Reference::referent.

But it seems referent() does something rather wrong. It's doing a strong
access, and that just happens to "work" because the places calling it are
code paths that are only used when a strong access doesn't do anything
special. I think those places ought to instead be using ON_UNKNOWN_OOP_REF
and AS_NO_KEEPALIVE. I'm making some changes there and changing jvm.cpp
accordingly.

@mlbridge
Copy link

@mlbridge mlbridge bot commented Oct 8, 2020

Mailing list message from Kim Barrett on hotspot-gc-dev:

On Oct 5, 2020, at 8:22 PM, Mandy Chung <mchung at openjdk.java.net> wrote:

Can you add a unit test in `test/jdk/java/lang/ref` that serves as a basic API test for this new `refersTo` API without
depending the WhiteBox API? test/hotspot/jtreg/gc` test serves as a more white-boxed type and comprehensive test.

Adding test/jdk/java/lang/ref/ReferenceRefersTo.java

src/java.base/share/classes/java/lang/ref/Reference.java line 361:

359:
360: /* Type-erased implementation of refersTo(). */
361: native boolean refersTo0(Object o);

Is there any reason why it can't use the parameter type like `refersTo0(T o)`? Same for `PhantomReference::refersTo0(T
o)`

I incorrectly assumed there would be a problem with determining the type of
the C function that implements the native method, but it seems the type
parameter gets erased to Object automatically. So the signature taking
Object is really just truth in advertising, whereas using the parameterized
type is kind of lying (even more so than is usual for Java generics). There
is also Daniel's question to consider. So I haven't made any change here
yet.

src/hotspot/share/include/jvm.h line 334:

332: */
333: JNIEXPORT jboolean JNICALL
334: JVM_PhantomReferenceRefersTo(JNIEnv *env, jobject ref, jobject o);

Instead of a separate `PhantomReference::refersTo0` native method (and new PhantomReference.c), an alternative approach
could be to detect if `ref` is an instance of `PhantomReference` or other `Reference` and get the referent accordingly.

See response to Roger's question about refersTo0.

@mlbridge
Copy link

@mlbridge mlbridge bot commented Oct 8, 2020

Mailing list message from Kim Barrett on hotspot-gc-dev:

On Oct 5, 2020, at 9:15 PM, Roger Riggs <rriggs at openjdk.java.net> wrote:
src/java.base/share/classes/java/lang/ref/PhantomReference.java line 66:

64: @OverRide
65: native final boolean refersTo0(Object o);
66:

How does the existence of this method help future intrinsification?
If it was called somewhere it would be clearer.
Perhaps a comment now or later would help explain its function.

public final refersTo(T o) calls refersTo0(o)

We have package private:
native boolean Reference::refersTo0(Object)
final native boolean PhantomReference::refersTo0(Object)

The reason for this split has to do with details in the VM support, in
particular C2 intrinsification.

For the native method support we don't need this split. The original
version from back in April just had a single native method in Reference. (It
did have native refersTo0, but that was my not realizing native methods
could have a parameterized type that would get type-erased automatically;
see response to Mandy.) That native method performed a runtime check on the
ReferenceType of the reference's klass to determine what to do. (See below.)

Phantom references need to be treated differently from stronger "weak"
reference types, because phantom references are weaker than finalization, so
might not be cleared when a stronger reference to the same object is
cleared. For collectors with STW reference processing this doesn't make
much difference (the referent is either cleared or not), but making this
distinction correctly is necessary for concurrent reference processing.

The Access API that provides the interface to the GC has support for
"unknown" referent accesses that perform a runtime lookup. This is
supported in C++ code, but not in the various Java code processors
(interpreter and compilers). We didn't previously need to support that case
for Java because the only place where it mattered was accessing the referent
of a PhantomReference, and that is blocked by PhantomReference::get that
always returns null.

For refersTo the intent is to have the interpreter and C1 use the native
method, but have C2 have special knowledge for it. We could add support for
the "unknown" reference type to C2, but that's a substantial amount of work,
and only useful for this one place. Or we can take the same approach as for
get(), i.e. have a second method on PhantomReference so that calls that can
select the appropriate method at compile time (usually the case) can have
distinct intrinsics for the two cases.

By having these intrinsifiable native methods be package private we can have
the public refersTo API function be final, while also preventing any further
overrides by classes not in the same package.

I'll try to come up with some commentary.

@mlbridge
Copy link

@mlbridge mlbridge bot commented Oct 8, 2020

Mailing list message from Kim Barrett on hotspot-gc-dev:

On Oct 6, 2020, at 5:22 AM, Daniel Fuchs <dfuchs at openjdk.java.net> wrote:

On Tue, 6 Oct 2020 00:04:11 GMT, Mandy Chung <mchung at openjdk.org> wrote:

src/java.base/share/classes/java/lang/ref/Reference.java line 361:

359:
360: /* Type-erased implementation of refersTo(). */
361: native boolean refersTo0(Object o);

Is there any reason why it can't use the parameter type like `refersTo0(T o)`? Same for `PhantomReference::refersTo0(T
o)`

But should `refersTo` takes a `T`? I mean - if I have a `Reference<?> ref`, the compiler will not let me query
`ref.refersTo(o)`; is this what we want?

I'm going to have to defer to the Java language experts on this question. Mandy? Alan?

@RogerRiggs
Copy link
Contributor

@RogerRiggs RogerRiggs commented Oct 8, 2020

@mlbridge
Copy link

@mlbridge mlbridge bot commented Oct 8, 2020

Mailing list message from Kim Barrett on hotspot-gc-dev:

On Oct 8, 2020, at 10:02 AM, Roger Riggs <rriggs at openjdk.java.net> wrote:
At what point would the @IntrinsicCandidate annotation be added to the
refersTo0 methods?
it would be useful documentation even if it is not needed for the mechanism.

I now have a patch to implement the C2 intrinsic, mostly provided by eosterlund.
It is very nearly completely separate from the current PR; the only point of contact
is adding the @IntrinsicCandidate annotations. Also, the intrinsic needs review
by pretty much a completely different set of people (compiler folks) than the
current PR.

Because of that, I decided to hold off and keep it separate, but I intend to make
a PR for it shortly after the current one is integrated. Also, the intrinsic needs
review by pretty much a completely different set of people (compiler folks) than
the current PR.

I don?t know what happens if one annotates as an intrinsic candidate with no
intrinsic implementation. I expect nothing much. But I originally didn?t want to
do that because it wasn?t initially clear when someone would have time for it,
and I didn?t want to be misleading about the performance in the meantime.

} else if (ref.refersTo(unexpectedValue)) {
fail(kind + " refers to unexpected value");
Copy link
Contributor

@pliden pliden Oct 12, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

@kimbarrett kimbarrett Oct 12, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A possible failure mode is to implement the predicate incorrectly, perhaps flipping the sense of the predicate. This may help notice such. It's distinct from the null test, since null is a special value that might be handled differently.

private static final class TestObject {
public final int value;

public TestObject(int value) {
this.value = value;
}
}
Copy link
Contributor

@pliden pliden Oct 12, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

@kimbarrett kimbarrett Oct 12, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd intended to use the value field when printing failure messages, but that became a little messy and probably not worth the trouble in the face of possible null values. So yeah, just using Object is fine; I'll make that change.

expectNotValue(testWeak2, testObject3, "testWeak2");
expectValue(testWeak3, testObject3, "testWeak3");
Copy link
Contributor

@pliden pliden Oct 12, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

@kimbarrett kimbarrett Oct 12, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think so. For example,
expectNotCleared(testWeak1, "testWeak1")
is not testing the same thing as
expectValue(testWeak1, testObject1, "testWeak1")
If the implementation is correct they will indeed always be consistent. But they could be different with an incorrect implementation.

Copy link
Contributor

@pliden pliden Oct 12, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't that just depend on how you implement expectValue()/expectCleared()? You could for example implement expectValue()/expectCleared() to be supersets of expectNotCleared()/expectNotValue(), which I think would make this easier to read. Something like this:

private static void expectValue(...) {
    if (ref.refersTo(null)) {
        fail("expected " + which + " to not be cleared");
    }
 
    if (!ref.refersTo(value)) {
        fail(which + " refers to unexpected value");
    }
}

private static void expectCleared(...) {
    if (ref.refersTo(new TestObject(5))) {
        fail(which + " refers to unexpected value");
    }

    if (!ref.refersTo(null)) {
        fail("expected " + which + " to be cleared");
    }
}


/*
* @test
* @summary Basic functional test of Reference.refersTo.
Copy link
Member

@mlchung mlchung Oct 12, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

private static final <T extends Reference>
void test(T ref,
Copy link
Member

@mlchung mlchung Oct 12, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

fail(kind + "refers to null");
} else if (!ref.refersTo(expectedValue)) {
fail(kind + " doesn't refer to expected value");
} else if (ref.refersTo(unexpectedValue)) {
Copy link
Member

@mlchung mlchung Oct 12, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@openjdk
Copy link

@openjdk openjdk bot commented Oct 20, 2020

@kimbarrett the issue for this pull request, JDK-8188055, already has an approved CSR request: JDK-8241029

@openjdk
Copy link

@openjdk openjdk bot commented Oct 20, 2020

@kimbarrett This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8188055: (ref) Add Reference::refersTo predicate

Reviewed-by: mchung, pliden, rriggs

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 68 new commits pushed to the master branch:

  • 5d1397f: 8254282: Add Linux x86_32 builds to submit workflow
  • 7ba6a6b: 8251158: Implementation of JEP 387: Elastic Metaspace
  • 5fedfa7: 8251271: C2: Compile::_for_igvn list is corrupted after RenumberLiveNodes
  • 98ec4a6: 8254805: compiler/debug/TestStressCM.java is still failing
  • 355f44d: 8254994: [x86] C1 StubAssembler::call_RT, "call_offset might not be initialized"
  • 0a75b37: 8254776: Remove unimplemented LowMemoryDetector::check_memory_usage
  • b65dcfa: 8197981: Missing return statement in __sync_val_compare_and_swap_8
  • 5b51085: 8254997: Remove unimplemented OSContainer::read_memory_limit_in_bytes
  • c87cdf7: 8254029: ObjectMonitor cleanup/minor bug-fix changes extracted from JDK-8253064
  • 7a580ca: 8255027: Problem list for Graal test gc/stress/TestStressG1Humongous.java
  • ... and 58 more: https://git.openjdk.java.net/jdk/compare/038f58d4f0782f885547d2b052a47fdf2229cfe3...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

@openjdk openjdk bot added the hotspot-gc label Oct 20, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Oct 20, 2020

@kimbarrett
The hotspot-gc label was successfully added.
The core-libs label was already applied.

@openjdk
Copy link

@openjdk openjdk bot commented Oct 20, 2020

@kimbarrett the issue for this pull request, JDK-8188055, already has an approved CSR request: JDK-8241029

@openjdk
Copy link

@openjdk openjdk bot commented Oct 20, 2020

@kimbarrett The hotspot-gc label was already applied.
The core-libs label was already applied.

@mlbridge
Copy link

@mlbridge mlbridge bot commented Oct 20, 2020

Mailing list message from David Holmes on hotspot-gc-dev:

On 20/10/2020 5:01 pm, Kim Barrett wrote:

On Oct 20, 2020, at 2:09 AM, David Holmes <david.holmes at oracle.com> wrote:

On 20/10/2020 3:26 pm, Kim Barrett wrote:

On Tue, 20 Oct 2020 03:25:45 GMT, Mandy Chung <mchung at openjdk.org> wrote:

@kimbarrett your reworded text is okay. I think "if it initially had some other referent value" can be dropped.

For a `Reference` constructed with a `null` referent, we can clarify in the spec that such reference object will never
get cleared and enqueued. I suggest to file a separate issue to follow up.
I don't think that clause can be dropped, because of explicit clearing (by clear() or enqueue()) rather than by the
GC. If the reference was constructed with a null referent, ref.refersTo(null) cannot tell whether ref.clear() has been
called.

I think that can be addressed by considering a Reference created with a null referent to be immediately cleared.

I think if it?s treated as immediately cleared then it should also be immediately enqueued. But immediate

Mandy's comment implied that references with a null referent never get
enqueued. Otherwise when would they get enqueued? There would be nothing
to trigger it.

David
-----

enqueue has problems; it can?t be done by the Reference constructor because the derived constructors
have not yet completed, and exposing the possibly incompletely constructed object to the queue processor
is problematic. And if it can?t be done by the constructor, it?s not clear who would do it.

But the more we discuss this the more I think allowing an initial null referent was a mistake in the first place. :(

I agree, but here we are. Very hard to know what the compatibility impact of changing that would be.

@mlbridge
Copy link

@mlbridge mlbridge bot commented Oct 20, 2020

Mailing list message from Kim Barrett on hotspot-gc-dev:

On Oct 20, 2020, at 3:21 AM, David Holmes <david.holmes at oracle.com> wrote:

On 20/10/2020 5:01 pm, Kim Barrett wrote:

On Oct 20, 2020, at 2:09 AM, David Holmes <david.holmes at oracle.com> wrote:

I think that can be addressed by considering a Reference created with a null referent to be immediately cleared.
I think if it?s treated as immediately cleared then it should also be immediately enqueued. But immediate

Mandy's comment implied that references with a null referent never get enqueued. Otherwise when would they get enqueued? There would be nothing to trigger it.

You said ?immediately cleared?; that?s the trigger. Mandy said ?never cleared or enqueued?, which is different.

@mlbridge
Copy link

@mlbridge mlbridge bot commented Oct 20, 2020

Mailing list message from David Holmes on hotspot-gc-dev:

On 20/10/2020 5:51 pm, Kim Barrett wrote:

On Oct 20, 2020, at 3:21 AM, David Holmes <david.holmes at oracle.com> wrote:

On 20/10/2020 5:01 pm, Kim Barrett wrote:

On Oct 20, 2020, at 2:09 AM, David Holmes <david.holmes at oracle.com> wrote:

I think that can be addressed by considering a Reference created with a null referent to be immediately cleared.
I think if it?s treated as immediately cleared then it should also be immediately enqueued. But immediate

Mandy's comment implied that references with a null referent never get enqueued. Otherwise when would they get enqueued? There would be nothing to trigger it.

You said ?immediately cleared?; that?s the trigger. Mandy said ?never cleared or enqueued?, which is different.

Mandy said never enqueued and that is what I would expect even if
considered "immediately cleared". A reference set with null is never
cleared by the GC, nor ever enqueued, but it is implicitly and trivially
"cleared" at construction.

"cleared" is just a synonym for "referent == null".

David

@mlchung
Copy link
Member

@mlchung mlchung commented Oct 20, 2020

Mandy's comment implied that references with a null referent never get enqueued. Otherwise when would they get enqueued? There would be nothing to trigger it.

Sorry I should have been clearer. What I try to say is that Reference(null)
will never be cleared and enqueued by GC since its referent is null.

Kim is right that Reference(null) can be explicitly cleared and enqueued
via Reference::enqueue. Reference::clear on such an "empty" reference
object is essentially a no-op. Whoever creates an "empty" reference would
not intend to be cleared.

But the more we discuss this the more I think allowing an initial null referent was a mistake in the first place. :(

I agree, but here we are. Very hard to know what the compatibility impact of changing that would be.

There are existing use cases depending on Reference(null) for example as a special
instance be an empty reference or the head of a doubly-linked list of references.
This was discussed two years ago [1].

[1] https://mail.openjdk.java.net/pipermail/core-libs-dev/2018-July/054325.html

@kimbarrett
Copy link
Author

@kimbarrett kimbarrett commented Oct 21, 2020

David, Mandy, and I discussed the wording in refersTo javadoc and reached a consensus that is reflected in 3a15b6a.

Copy link
Member

@dholmes-ora dholmes-ora left a comment

Update looks good. Need to reflect the change in the CSR.

Thanks.
David

@magicus
Copy link
Member

@magicus magicus commented Oct 21, 2020

Build changes look good.

Copy link
Contributor

@AlanBateman AlanBateman left a comment

The API looks good, thanks for getting this in.

@kimbarrett
Copy link
Author

@kimbarrett kimbarrett commented Nov 4, 2020

/integrate

@openjdk openjdk bot closed this Nov 4, 2020
@openjdk openjdk bot added integrated and removed ready rfr labels Nov 4, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Nov 4, 2020

@kimbarrett Since your change was applied there have been 2 commits pushed to the master branch:

  • ab9192e: 8255681: print callstack in error case in runAWTLoopWithApp
  • c7a2c24: 8255797: ciReplay: improve documentation of replay file syntax in parser

Your commit was automatically rebased without conflicts.

Pushed as commit 6023f6b.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

openjdk-notifier bot referenced this issue Nov 4, 2020
Reviewed-by: mchung, pliden, rriggs, dholmes, ihse, smarks, alanb
@amaembo
Copy link
Contributor

@amaembo amaembo commented Nov 4, 2020

Hello!

As an IDE developer, I'm thinking about IDE inspection that may suggest the new method. My idea is to suggest replacing every ref.get() == obj with ref.refersTo(obj). Is this a good idea or there are cases when ref.get() == obj could be preferred? What do you think?

@kimbarrett
Copy link
Author

@kimbarrett kimbarrett commented Nov 4, 2020

Thanks to a whole host of folks for reviews and comments.

@kimbarrett
Copy link
Author

@kimbarrett kimbarrett commented Nov 4, 2020

Hello!

As an IDE developer, I'm thinking about IDE inspection that may suggest the new method. My idea is to suggest replacing every ref.get() == obj with ref.refersTo(obj). Is this a good idea or there are cases when ref.get() == obj could be preferred? What do you think?

Those have different behaviors when ref's class overrides get. Sometimes that might be intentional (PhantomReference, where get blocks access to the referent, and SoftReference, where get may update heuristics for recent accesses delaying GC clearing). But if some further subclass overrides get for some reason, such a change might not be appropriate.

@kimbarrett kimbarrett deleted the refersto branch Nov 4, 2020
@mlchung
Copy link
Member

@mlchung mlchung commented Nov 4, 2020

Checking if a reference has been cleared i.e. ref.get() == null or ref.get() != null may benefit with IDE giving a hint.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build core-libs hotspot hotspot-gc integrated