RFR: 8194312: Support parallel and concurrent JNI global handle processing

[Erik Österlund]

Hi Kim,

I have already seen earlier incarnations of this code so I only have a 
few comments regarding things that were not in that incarnation.

src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/gc/shared/OopStorage.java:

The Java implementation of OopStorage for the SA agent seems a bit empty 
and full of TODO comments to actually implement something. It seems like 
quite a bit of the other changes to the SA assumes that this actually 
does something. For example there are changes to use OopStorage oopsDo 
instead of JNIHandleBlock oopsDo, which I presume means there has to be 
some oopsDo implementation in place for OopStorage.
Is the intention to implement that later on in a separate RFE?

src/hotspot/share/gc/shared/oopStorage.hpp:

The OopStorage::oops_do functions accept some kind of OopClosure and 
performs iteration with it. I'm curious though why it takes the specific 
type of OopClosure as a template parameter, passes it around, and 
eventually discards the information without making use of it? I was 
thinking perhaps the intention was to use the closure type information 
to devirtualize the do_oop() call, but that does not appear to be the 
case. To do that you would have to in OopStorage::OopFn::operator()() 
perform a qualified _cl->Closure::do_oop() call instead of _cl->do_oop() 
(and possibly take some care to prevent abstract oop closure types like 
OopClosure or ExtendedOopClosure from being devirtualized, which would 
be awkward (doable with some EnableIf<!IsSame<Closure, 
OopClosure>::value>::type overload, or something like that).
In other words, it seems like either this template parameter is 
unnecessary and you could just replace uses of it with OopClosure, or it 
should be made useful, e.g. by devirtualizing the calls or something. 
Same goes for the IsAliveClosure parameter for weak_oops_do.

src/hotspot/share/gc/shared/oopStorage.cpp:

I recall in the pre-review we had some discussion about whether the 
infrastructure for supporting early iteration termination was necessary 
or not. You had me convinced that JNI would need it for something. Is 
this still the case, or has that use case been revised? Reading through 
the new JNI code, I could not find uses of 
OopStorage::iterate_safepoint() from the JNI code, and hence no uses of 
early termination. Having said that, it does look rather simple now with 
only the sequential case supporting early termination.
However, if there is indeed no use any longer for OopStorage::iterate() 
and hence the only publicly available tool for early iteration exit, I 
wonder if perhaps it should be reconsidered if we want to remove that 
infrastructure. Or will there be other use cases down the road? 
Currently I find it at least a bit confusing that, e.g. 
OopStorage::iterate_safepoint() expects a function of type F that 
returns bool while OopStorage::BasicParState::iterate() expects a 
function of  type F that returns void. For the 
OopStorage::iterate_safepoint() case it is clearly described in the 
documentation, in the OopStorage::BasicParState::iterate() case I did 
not find it clear that this F is a different F to the safepoint one.
I guess I propose either removing early termination support alltogether 
if nobody is using it anyway (and hence having the F function type 
consistently return void), or documenting clearly that the sequential 
and parallel iteration functions expect different function types - one 
with bool return type and one with void return type, depending on 
whether early termination is supported or not.

Apart from the above minor remarks, I am delighted to see these changes. 
I am extra delighted to see deleted_handle() disappear. I am also 
delighted to see the new simplified synchronization for parallel 
iteration - that worked out nicely. Thank you for building a good oop 
storage solution.

Thanks,
/Erik

On 2018-01-02 22:41, Kim Barrett wrote:
> Please review this change to the implementation of JNI global and weak
> global handles, providing infrastructure for parallel and concurrent
> processing of such handles.
>
> This change introduces OopStorage, a data structure for managing
> off-heap references to objects allocated in the Java heap. JNI global
> and weak global handles are reimplemented using OopStorage, rather
> than using JNIHandleBlock. (JNI local handles continue to use
> JNIHandleBlock; the lifetime management and concurrency issues are
> very different for local vs global handles.)
>
> This change does not change any GCs to use the new parallel and
> concurrent capabilities, only laying the foundations for doing so.
>
> Other uses of OopStorage are also being considered, in the context of
> runtime latency improvements for ZGC and other collectors. The idea is
> to change some (often weak) oop references in runtime data structures
> to OopStorage-managed handles, simplifying the GC processing and
> avoiding (potentially concurrent) GC walks of runtime data structures.
>
> As a side effect, this enhancement fixes these bugs:
> 8174790: Race adding (weak) global JNI handles and determining type of handle
> 8176454: Performance: jweak references not suitable for robust cache architecture
>
> Some things not addressed by this change include:
>
> - 8194309: JNI handle allocation failure not reported correctly
> For now, the pre-existing vm_exit_out_of_memory behavior is retained.
>
> - Updating JNIHandles to use the new Access protocol.
>
> - Serviceability Agent updates for OopStorage and JNI usage.  Just
> enough has been done to allow existing SA tests to "pass".  This will
> be addressed as a followup.
>
> CR:
> https://bugs.openjdk.java.net/browse/JDK-8194312
>
> Webrev:
> http://cr.openjdk.java.net/~kbarrett/8194312/open.00/
>
> Testing:
> Mach5 hs-tier1 through hs-tier5, jdk-nightly
>