RFR: 8166188: G1 Needs pre barrier on dereference of weak JNI handles

[Per Liden]

Hi Kim,

On 2017-01-19 06:31, Kim Barrett wrote:
> Please review this change to jweak to support G1.
>
> The problem being addressed is that, when using G1, dereferencing a
> jweak must ensure the obtained referent is ensured live, just as for
> other weak reference types.  This has always been a requirement, and
> failure to do so appears to be a since-day1 G1 bug.
>
> There are two categories of places that need to address this issue:
> JNIHandles::resolve and friends, and returning a jobject from native
> code to Java.  In both of these places the jobject whose contained oop
> is being extracted might really be a jweak.  jweak is
> representationally indistinguishable from jobject; jweak is just a
> typedef for jobject.  The documentation says a jweak can be used
> anywhere a jobject is permitted, making that type equivalence
> necessary for a C API.  (A C++ API might be able to have distinct
> types via inheritance, though would still need a method for
> distinguishing them at runtime.  But since JNI is a C API...).
>
> The basic approach being taken is to "tag" jweaks by setting the low
> order bit (recall that jweak == jobject == _jobject*), in order to
> distinguish them from non-jweak jobjects.  This tagging is only being
> done when the selected GC needs the distinction, e.g. G1.  This gives
> us a representational difference between jobject and jweak on which to
> base the different behavior, hiding that distinction behind the
> existing API.

I'd like to suggest that we always tag jweaks, regardless of which GC is 
used. I can't see any performance issues to be afraid of here, but I can 
see a number of advantages with a unified jweak representation/handling, 
like avoiding #ifdef INCLUDE_ALL_GCS noise and reducing the number of 
code paths to test/debug.

cheers,
Per

>
> JNIHandles::resolve and friends are changed to unconditionally strip
> off any potential weak tag when translating from jobject to to oop*.
> That's cheaper than testing for the conditional use of tagging and
> then stripping.  Also stripped when deleting JNI handles.
>
> TemplateInterpreterGenerator::generate_native_entry and
> SharedRuntime::generate_native_wrapper are changed to conditionally
> emit code to apply the G1 pre-barrier to the value obtained from a
> jobject result, when the result is tagged as a jweak, which only
> occurs when using G1.
>
> For arm32/arm64, this required moving the g1_write_barrier_pre
> definitions from InterpreterMacroAssembler to MacroAssembler.  Also
> moved g1_write_barrier_post, for consistency.
>
> Note: I've not tested the aarch64 changes; I'll need someone with
> access to that platform to test.
>
> Note: I've not implemented this change for ppc or s390; I'll need
> someone else to deal with those platforms.  (It's been a very long
> time since I wrote any ppc assembly code, and I've never seen an
> s390.)
>
> Note: I've done a minimal change for zero.  Code elsewhere suggests
> zero doesn't support G1, and I followed that.
>
> And finally, JNIHandles::make_weak_global conditionally tags the
> result.
>
> CR:
> https://bugs.openjdk.java.net/browse/JDK-8166188
>
> Webrev:
> http://cr.openjdk.java.net/~kbarrett/8166188/hotspot.02/
>
> In addition to the above webrev, I've also included separate webrevs
> for a test of this change, and for a whitebox extension needed for
> that test.  The whitebox extension is my in-progress work on
> JDK-8169517, which is an enhancement targeted for JDK 10, and is not
> part of this change.  Since the test depends on that whitebox
> extension, the test can't be included with the change either, and will
> be deferred to JDK 10.  But I'm including webrevs for both here, for
> for reference by reviewers, and for testing on the platforms I don't
> have access to.
>
> http://cr.openjdk.java.net/~kbarrett/8166188/test.03/
> http://cr.openjdk.java.net/~kbarrett/8166188/whitebox/hsroot.01/
> http://cr.openjdk.java.net/~kbarrett/8166188/whitebox/hotspot.01/
>
> Testing:
> All hotspot jtreg tests run locally (Linux x86_64).
> In particular:
> - gc/TestReturnJNIWeak (new)
> - runtime/SameObject (uses NewWeakGlobalRef)
>
> Ad hoc hotspot nightly test on Oracle supported platforms.  This
> includes some closed tests that use NewWeakGlobalRef.
>