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

[Kim Barrett]

> 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/sharedRuntime_x86_32.cpp.udiff.html
> http://cr.openjdk.java.net/~kbarrett/8166188/hotspot.04/src/cpu/x86/vm/sharedRuntime_x86_64.cpp.udiff.html
> http://cr.openjdk.java.net/~kbarrett/8166188/hotspot.04/src/cpu/x86/vm/templateInterpreterGenerator_x86.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 sharedRuntime_x86.cpp.   So in the x86 case, there are 3.  I think these should be refactored into a function in macroAssembler_x86.cpp, something like unpack_native_oop_result(). 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::put_obj((oop)handle, _value, size); // Updates size.
> 
> 
> I think as a future cleanup, we should make JNITypes::put_obj take (intptr_t) 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 value_state uint when it's being stored into uchar?
> 
> + static const uint value_state_primitive = 0;
> 
> 
> Could they be enums near where _value_state_buffer 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 resolve_impl 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.