RFR: 8215451: JNI IsSameObject should not keep objects alive

[Per Liden]

Hi Kim,

On 12/17/18 1:12 PM, Kim Barrett wrote:
>> On Dec 17, 2018, at 3:00 AM, Per Liden <per.liden at oracle.com> wrote:
>>
>> On 12/17/18 5:59 AM, Kim Barrett wrote:
>>>>
>>> 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 thought that might be what was going on, but wanted to make sure.
> 
>>
>>> I think there might be a race that makes this not work for ZGC.
>>> […]
>>
>> 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.
> 
> Ah, I see. I'm not yet all that familiar with some (lots!) of the ZGC
> code, and lost my way when previously looking at how AS_NO_KEEPALIVE
> worked. I now see that it goes through ZBarrier::remap, where if we're
> in a relocating phase then both resolves will be forwarded, else
> neither if not relocating. And the phase can't change between those
> calls.
> 
> OK, change looks good.

Thanks for reviewing!

> 
> I was going to suggest the explicit 0 decorator values seemed like an
> abstraction violation, and should instead be using the named "empty"
> decorator value. But I see that's called "INTERNAL_EMPTY". That seems
> like a (separate) bug.
> 

I had the exact same thought and I talked to Erik about it. We agreed 
that we should rename INTERNAL_EMPTY to something less "internal". How 
about DECORATORS_NONE?

cheers,
Per