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.
