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

Kim Barrett kim.barrett at oracle.com
Sat Feb 11 00:30:38 UTC 2017

On Feb 10, 2017, at 5:49 AM, Thomas Schatzl <thomas.schatzl at oracle.com> wrote:

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

Thanks for looking at this. I’ll have a new webrev containing changes discussed here and elsewhere soon; need to do some testing first.

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

Yes, I noticed that yesterday. Thanks.

- jniHandles.hpp: it might be useful to explain the externalguard template parameter to the new guardvalue/resolveimpl methods.

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

I've added some commentary. The template parameter is just whether these helpers are being called from resolve_external_guard or not. That variant resolves some erroneous cases to NULL, rather than treating them as (possibly unchecked) errors.

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

I expect making external_guard an ordinary bool argument and letting the compiler constant fold would produce equivalent code. I went with a template parameter because, semantically, my intent is that the value must be a constant, and making it a template parameter enforces that.

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

Separate issue. File an RFE?

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

— TBD

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

I.e. one thread adding a global handle, another thread calling e.g. jniGetObjectRefType? That may be another issue. Same with the accesses to weakglobalhandle value.

Separate issue. File a bug?

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

Agreed. Done.

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

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

Typos fixed. signature is the name of the variable, so I prefer not capitalizing.

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

Already added. Still not totally convinced this is appropriate.

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

Done. Also eliminated p variable, which I don’t think helps readability. And cleaned up the comments nearby.

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

Added more description.

- templateInterpreterGeneratorsparc.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, storeresult); delayed()->ldptr(O0, 0, O0); // <--- this one I think is a guaranteed unaligned load if O0 contains a jweak. ldptr(O0, -JNIHandles::weaktagvalue, O0);

The code is correct, there is no unaligned access.

In the brx, the second argument is true. That’s the annul flag. That means the instruction in the delay slot will only be executed if the branch is taken, and will be treated as a nop if the branch is not taken. If the annul flag is false then the instruction in the delay slot will be executed whether the branch is taken or not.

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 sharedRuntimesparc.cpp. - I am also not sure why some of the code generation methods have the STATICASSERT(JNIHandles::weaktagmask == 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.

In non-ARM code, there’s no dependency on the size of the tag. The ARM/AARCH64 code is using a tbz instruction, test a single bit (specifically bit 0 in this case, see the second argument) and branch if zero. So we’re assuming the tag field is one bit wide and at bit zero, e.g. mask == 1. Hence the static assert.

More information about the hotspot-dev mailing list