RFR: 8215451: JNI IsSameObject should not keep objects alive

[Per Liden]

Hi Kim,

On 12/17/18 5:59 AM, Kim Barrett wrote:
>> On Dec 15, 2018, at 5:41 AM, Per Liden <per.liden at oracle.com> wrote:
>>
>> JNI IsSameObject should resolve the JNIHandles its comparing without keeping the objects they point to alive. Otherwise, checking if a jweak is reachable by doing IsSameObject(jweak, NULL) will be counter productive, as it will make sure that jweak stays alive.
>>
>> This is currently only a problem when using ZGC, which does concurrent jweak processing.
>>
>> This was found by the "test/hotspot/jtreg/serviceability/jvmti/HeapMonitor/MyPackage/HeapMonitorThreadTest.java" test, which keeps a list of jweaks, which it will periodically iterate over and do IsSameObject(jweak, NULL) to determine if any of them have been cleared. The end result is the these jweaks will never be cleared and the heap eventually gets full and an OOME is thrown.
>>
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8215451
>> Webrev: http://cr.openjdk.java.net/~pliden/8215451/webrev.0
>>
>> /Per
> 
> This is applying AS_NO_KEEPALIVE for both strong and weak jobjects.
> I'm not sure that's intentional.  The CR and RFR only talk about
> jweaks.

Sorry, the RFR description was a bit sloppy.

Not applying it on jobjects is less problematic, but there is still a 
tiny window where it's beneficial. Imagine that ZGC starts marking, the 
application does IsSameObject(handle, something) followed by 
DeleteGlobalRef(handle), before the strong marking of jobjects has 
begun. By using AS_NO_KEEPALIVE we will allow the object the handle 
points to to die this GC cycle, rather than the next one (assuming it's 
the only strong reference to that object).

> 
> I think there might be a race that makes this not work for ZGC.
> 
> I think (but perhaps I'm misremembering?) that only Shenandoah does
> anything interesting for Access::equals, with all other collectors in
> the JDK (including ZGC) just resolving down to an == on the arguments.
> If that's incorrect then the rest of this comment may be wrong.
> 
> Consider we have two jobject arguments, A and B, neither resolved (so
> neither contains a "good" value), and both referring to the same
> object that is not yet marked.
> 
> (1) resolve_no_keepalive(A) => the unmarked (unforwarded) value.
> (2) Some other thread resolves B, marking it and making B's value good.
> (3) resolve_no_keepalive(B) => the marked (forwarded) value.
> (4) result of (1) == result of (2) => false.

This is not what happens in this scenario. ZGC always maintains the 
to-space invariant, so resolve() and resolve_no_keepalive() always 
return a "good" pointer. The AS_NO_KEEPALIVE only dictates if the object 
should be marked or not, and how it's healed, but it never affects what 
the barrier returns.

Note that we've been using this scheme to compare non-strong refs for a 
long time now, for example, when comparing Strings in StringTable 
without resurrecting them. So this isn't something new.

> 
> Obviously this isn't a problem for IsSameObject(jweak, NULL). I think
> what we really have is JNI weak having the same issue as
> j.l.r.Reference and likely needs a similar solution, e.g. JDK-8188055.

Agree, without my patch, IsSameObject(jweak, obj) has the same 
resurrection problem as Reference.get() == obj, which is why we would 
like to have a Reference.isReferenceTo().

> The proposed Reference.isReferenceTo doesn't have the above race.

It doesn't have this race because such an intrinsic would do what my 
IsSameObject() patch is doing. Conceptually it would look like this:

bool Reference::is_referring_to(oop obj) {
   oop referent = 
HeapAccess<ON_WEAK_OOP_REF|AS_NO_KEEPALIVE>::oop_load(referent_addr);
   return oopDesc::equal(referent, obj);
}

cheers,
Per

> 
> Responding to David - the resolve keeps the jweak's referent alive for
> the current collection cycle. It could still die in some later
> collection cycle, unless the described iteration happens again.
> JDK-8188055 is the same problem for j.l.r.Reference.
> 
>