RFR: 8166188: G1 Needs pre barrier on dereference of weak JNI handles (original) (raw)

Thomas Schatzl thomas.schatzl at oracle.com
Fri Feb 10 10:49:50 UTC 2017

Hi Kim,

On Mon, 2017-02-06 at 20:03 -0500, Kim Barrett wrote:

Apologies for taking so long to respond to earlier comments.  I've been distracted by some other tasks, and it took a while to figure out what to do about the problem Mikael reported.

[...] CR: https://bugs.openjdk.java.net/browse/JDK-8166188 New webrevs: full: http://cr.openjdk.java.net/~kbarrett/8166188/hotspot.04

 - ReturnJNIWeak.java: please make sure that the test is not called with -XX:+ExplicitGCInvokesConcurrent. It will most likely fail in this case.

 - jniHandles.hpp: it might be useful to explain the external_guard template parameter to the new guard_value/resolve_impl methods.

This might be due to my unfamiliarity to the code, so skip this if you think it is "obvious" what this means.

From the callers of JNIHandles::resolve() I am actually not sure if the extra performance due to templatizing the parameter has any impact.

Again, your call.

 - it would be nice if JNIHandles::weak_oops_do(BoolObjectClosure, OopClosure) would have an assert_at_safepoint_or_JNI_lock_locked() - but that's probably another issue. Same with the _global_handles array.

 - jniHandles.hpp:58. Not sure why the "int" type specifier for weak_tag_value has an extra space in front of it.

 - isn't is_global_handle() thread-unsafe too? The assert uses JNIHandleBlock::chain_contains(), which iterates over the JNIHandles array unguarded, but it seems possible to add entries concurrently using e.g. make_global()?

I.e. one thread adding a global handle, another thread calling e.g. jni_GetObjectRefType?

That may be another issue. Same with the accesses to _weak_global_handle value.

 - javaCalls.hpp: in line 135 I would prefer if the two increment statements were put on separate lines. I looked over the second one a few times too many.

 - javaCalls.cpp:449: s/Invaild/Invalid/

 - javaCalls.cpp:526: s/matched/match/ ; maybe also upper case the initial word of the sentence.

 - javaCalls.cpp: somebody already asked for a comment for the t < os::vm_page_size() check.

 - in SignatureChekker::check_obj(), maybe put the declaration of the "bad" bool into the if-scope as it is never used outside.

 - I would prefer that the tagging mechanism would be described better in JNIHandles.hpp. "Low tag bit in jobject used to distinguish a jweak" seems not sufficient to explain what and why we do the tagging (or how it works). Particularly because some code (shareNativeWrapper.cpp) refers to an explanation in JNIHandles.hpp which is not there.

 - templateInterpreterGenerator_sparc.cpp, line 1523: am I correct that in case of jweaks, this generates a guaranteed unaligned load?

E.g. the sequence:

brx(Assembler::zero, true, Assembler::pt, store_result); delayed()->ld_ptr(O0, 0, O0);  // <--- this one I think is a guaranteed unaligned load if O0 contains a jweak. ld_ptr(O0, -JNIHandles::weak_tag_value, O0);

On some machines this may cause a SIGBUS afaik - depending on OS configuration/capabilities. I would prefer if the code would not attempt to be clever here.

Apparently the same for sharedRuntime_sparc.cpp.

 - I am also not sure why some of the code generation methods have the STATIC_ASSERT(JNIHandles::weak_tag_mask == 1u) and others not. (E.g. ARM32/64 code has it, others apparently not). I would kind of prefer to have it everywhere, but I am also fine with one e.g. in JNIHandles.cpp with a big fat comment.

Thanks,   Thomas

More information about the hotspot-dev mailing list