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

Kim Barrett kim.barrett at oracle.com
Fri Feb 10 23:05:10 UTC 2017

On Feb 9, 2017, at 9:34 PM, Coleen Phillimore <coleen.phillimore at oracle.com> wrote:

http://cr.openjdk.java.net/~kbarrett/8166188/hotspot.04/src/cpu/x86/vm/sharedRuntimex8632.cpp.udiff.html http://cr.openjdk.java.net/~kbarrett/8166188/hotspot.04/src/cpu/x86/vm/sharedRuntimex8664.cpp.udiff.html http://cr.openjdk.java.net/~kbarrett/8166188/hotspot.04/src/cpu/x86/vm/templateInterpreterGeneratorx86.cpp.udiff.html The code to unpack the oop result is the same at least for the x86 implementations, which unfortunately still have 32/64 bit variants for sharedRuntimex86.cpp. So in the x86 case, there are 3. I think these should be refactored into a function in macroAssemblerx86.cpp, something like unpacknativeoopresult(). It's not technically unboxing because boxing refers to making primitives into java.lang.Integer, etc objects. Sorry that this might be more work and tricky for the other platforms because I didn't diff them, but it would be a lot better. Maintaining separate copies of assembly code is a lot of work too. You can file an RFE/bug to clean this up if there was not a reason for the duplication because of ZBB.

I’m working on this. I’m calling the macroAssembler function resolve_jobject. x86 is easy. ARM64 (and ARM32 sharedRuntime) are also easy. ARM32 templateInterpreterGenerator is different, but maybe not for a good enough reason; still investigating that one. Sparc is hard, and I’m going to punt that one. I’ll probably leave the non-Oracle supported ports to their owners to decide whether to do something similar.

http://cr.openjdk.java.net/~kbarrett/8166188/hotspot.04/src/share/vm/prims/jni.cpp.udiff.html

Can you change 'l' to obj? never mind, I see why you picked 'l' but it looks like a 1.

Don’t blame me; I had nothing to do with that choice. :)

http://cr.openjdk.java.net/~kbarrett/8166188/hotspot.04/src/share/vm/runtime/javaCalls.hpp.udiff.html

I see what you've done. These are terrible casts. + JNITypes::putobj((oop)handle, value, size); // Updates size.

I think as a future cleanup, we should make JNITypes::putobj take (intptrt) or (jobject,Handle) instead of dealing with the oop class for CheckUnhandledOops. I didn't see any other callers except this one.

Yes, that’s the "followup cleanup bug” I was referring to.

Why is valuestate uint when it's being stored into uchar?

+ static const uint valuestateprimitive = 0;

Could they be enums near where valuestatebuffer is declared?

I had a reason in an earlier version of the code, but an enum makes sense now, so I’ll make that change.

http://cr.openjdk.java.net/~kbarrett/8166188/hotspot.04/src/share/vm/runtime/jniHandles.hpp.udiff.html

I don't think it's good to have to include +#include "gc/g1/g1SATBCardTableModRefBS.hpp"

in jniHandles.hpp header file. could you refactor resolveimpl to put the jweak case in the .cpp file? I don't think performance is going to be important for jweak. All this work for an indirection. With the template code, it's hard to see that's all it is. :(

Good idea. I’ll make that change.

http://cr.openjdk.java.net/~kbarrett/8166188/hotspot.04/test/runtime/jni/ReturnJNIWeak/ReturnJNIWeak.java.html

Someone else asked this question but how can you assume that GC cleared the weak reference? 115 System.gc(); 116 // Verify weak ref cleared as expected.

There are no longer any strong references…

That's all. The change looks good.

Thanks. I’ll have a new webrev out soon; need to finish per-review changes and retest first.

More information about the hotspot-dev mailing list