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

[Kim Barrett]

> 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.

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.

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?

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.

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.