RFR: 8195972: Refactor oops in JNI to use the Access API

[coleen.phillimore at oracle.com]

On 3/29/18 9:54 AM, Erik Österlund wrote:
> Hi Kim,
>
> On 2018-03-29 01:35, Kim Barrett wrote:
>>> On Mar 28, 2018, at 9:40 AM, Erik Österlund 
>>> <erik.osterlund at oracle.com> wrote:
>>>
>>> Hi Kim,
>>>
>>> I noticed that jobjects are now IN_CONCURRENT_ROOT in this patch. I 
>>> wonder if this is the right time to upgrade them to 
>>> IN_CONCURRENT_ROOT. Until there is at least one GC that actually 
>>> scans these concurrently, this will only impose extra overheads 
>>> (unnecessary G1 SATB-enqueue barriers on the store required to 
>>> release jobjects) with no obvious gains.
>>>
>>> The platform specific code needs to go along with this. I have a 
>>> patch out to generalize interpreter code. In there, I am treating 
>>> resolve jobject as a normal strong root. That would probably need to 
>>> change. It is also troubling that jniFastGetField shoots raw loads 
>>> into (hopefully) the heap, dodging all GC barriers, hoping that is 
>>> okay. I wonder if starting to actually scan jobjects concurrently 
>>> would force us to disable that optimization completely to be 
>>> generally useful to all collectors. For example, an 
>>> IN_CONCURRENT_ROOT load access for ZGC might require a slowpath. But 
>>> in jniFastGetField, there is no frame, and hence any code that runs 
>>> in there must not call anything in the runtime. Therefore, with 
>>> IN_CONCURRENT_ROOT, it is not generally safe to use jniFastGetField, 
>>> without doing... something about that code.
>>>
>>> I would like to hear your thoughts about this. Perhaps the intention 
>>> is just to take incremental steps towards being able to scan 
>>> jobjects concurrently, and this is just the first step? Still, I 
>>> would be interested to hear about what you think about the next 
>>> steps. If we decide to go with IN_CONCURRENT_ROOT now already, then 
>>> I should change my interpreter changes that are out for review to do 
>>> the same so that we are consistent.
>>>
>>> Otherwise, this looks great, and I am glad we finally have jni 
>>> handles accessorized.
>> With this change in place I think it should be straight-forward for G1
>> to do JNI global handle marking concurrently, rather than during a
>> pause.
>>
>> This change does come with some costs.
>>
>> (1) For G1 (and presumably Shenandoah), a SATB enqueue barrier when
>> setting a global handle's value to NULL as part of releasing the
>> handle.
>>
>> (2) For other collectors, selection between the above barrier and
>> do-nothing code.
>>
>> (3) For ZGC, a read barrier when resolving the value of a non-weak
>> handle.
>>
>> (4) For other collectors (when ZGC is present), selection between the
>> above barrier and do-nothing code.
>>
>> (1) and (2) are wasted costs until G1 is changed to do that marking
>> concurrently.  But the cost is pretty small.
>>
>> I think (3) and (4) don't apply to the jdk repo yet.  And even in the
>> zgc repo the impact should be small.
>>
>> All of these are costs that we expect to be taking eventually anyway.
>> The real costs today are that we're not getting the pause-time benefit
>> from these changes yet.
>
> Fair enough.
>
>> Even those (temporary) costs could be mitigated if we weren't forced
>> to use the overly generic IN_CONCURRENT_ROOT decorator, and could
>> instead provide more precise information to the GC-specific backends
>> (e.g. something like IN_JNI_GLOBAL_ROOT), letting each GC defer its
>> extra barrier work until the changes to get the pause-time benefits
>> are being made.
>
> Sure. But I'd like to avoid overly specific decorators that describe 
> the exact root being accessed, rather than its semantic properties, 
> unless there are very compelling reasons to do so.

Since the GC has code to process the roots as distinct things (roots on 
stack vs roots in ClassLoaderData vs roots in JNIHandles), maybe we do 
need to have a decorator to say which root it is.  Then the GC can 
figure out how it wants to process it: concurrently, as a strong root in 
a safepoint, etc.

This sort of decoration would make a lot more sense to me because I 
still haven't figured out what the GC does differently for 
IN_CONCURRENT_ROOT vs IN_ROOT or what RootAccess<> with nothing means.

Cutting and pasting in the runtime code isn't helping anymore! Also, I 
tried to review this code in jni, which is technically runtime code, and 
have a separate thread with Kim asking how he picked these decorators.

We're trying to reduce the roots/oop references from the runtime so 
eventually there should be a limited set, and we can remove some as we 
do this.

>> I'd forgotten about jniFastGetField.  This was discussed when Mikael
>> and I were adding the jweak tag support.  At the time it was decided
>> it was acceptable (though dirty) for G1 to not ensure the base object
>> was kept alive when fetching a primitive field value from it.
>>
>> http://mail.openjdk.java.net/pipermail/hotspot-dev/2017-March/026231.html 
>>
>>
>> I suspect that choice was driven by the difficulties you noted, and
>> knowing that something better to solve all our problems (Access!) was
>> coming soon :) Unfortunately, that (among other things here) really
>> doesn't work for ZGC, even though it seems okay for all the other
>> collectors, at least for now.  Any idea how important an optimization
>> jniFastGetField might be?  How bad would it be to turn it off for ZGC?
>
> I have no idea what the cost would be of turning it off. But I feel 
> more compelled to fix it so that we do not have to turn it off. Should 
> not be impossible, but can be done outside of this RFE.
>
>> For the interpreter, I think you are referring to 8199417?  I hadn't
>> looked at that before (I'll try to review it tomorrow).  Yes, I think
>> those should be using IN_CONCURRENT_ROOT too, so that eventually ZGC
>> can do JNI global marking concurrently.
>
> Yeah.
>
>> And there are two other pre-existing uses of IN_CONCURRENT_ROOT, both
>> of which seem suspicious to me.
>>
>> - In ClassLoaderData::remove_handle() we have
>>
>>      // This root is not walked in safepoints, and hence requires an 
>> appropriate
>>      // decorator that e.g. maintains the SATB invariant in SATB 
>> collectors.
>>      RootAccess<IN_CONCURRENT_ROOT>::oop_store(ptr, oop(NULL));
>>
>> But there aren't any corresponding IN_CONCURRENT_ROOT loads, nor is
>> the initializing store (CLD::ChunkedHandleList::add), which seems
>> inconsistent.  (To be pedantic, the initializing store should probably
>> be using the new RootAccess<AS_DEST_NOT_INITIALIZED> rather than a raw
>> store.)  Oh, the load is OopHandle::resolve; and I think OopHandle is
>> still pending accessorizing (and probably needs the access.inline.hpp
>> cleanup...).
>>
>> - In InstanceKlass::klass_holder_phantom() we have
>>
>>    return RootAccess<IN_CONCURRENT_ROOT | 
>> ON_PHANTOM_OOP_REF>::oop_load(addr);
>>
>> My understanding of it is that IN_CONCURRENT_ROOT is not correct
>> here.  I think this is similar to jweaks, where I only used
>> ON_PHANTOM_OOP_REF.
>

Both of these were cut and pasted, ie, they were guesses.   If they were 
IN_LOADER_DATA_ROOT (for both of these), IN_JNI_ROOT, or IN_THREAD_ROOT  
you can figure out how to apply barriers to these things.

You can fix these in a separate RFE.  Also, there's a store in oop* 
ClassLoaderData::ChunkedHandleList::add(oop o), which I think should be the

AS_DEST_NOT_INITIALIZED

case (had to cut/paste but this decoration makes sense from a 
documentation standpoint).

Kim's change looks correct as far as I can tell.  IN_CONCURRENT_ROOT is 
equivalent to IN_ROOT right now, isn't it?

Thanks,
Coleen
> Yes, that sounds like it should be fixed in separate RFEs.
>
> Your JNI handle changes look good.
>
> Thanks,
> /Erik