RFR(S): 8165489: Missing G1 barrier in Unsafe_GetObjectVolatile

[Kim Barrett]

> On Sep 12, 2016, at 1:09 PM, Doerr, Martin <martin.doerr at sap.com> wrote:
> 
> Hi Mikael,
> 
> thanks for reviewing.
> 
> I have created a new webrev according to your recommendations.
> "is_java_lang_ref_Reference_write" would be confusing because it is a read access.
> 
> Webrev is here:
> http://cr.openjdk.java.net/~mdoerr/8165489_G1_Unsafe/webrev.01/

For the record, I'm still planning to sponsor this.

------------------------------------------------------------------------------
src/share/vm/prims/unsafe.cpp 

is_java_lang_ref_Reference_access and ensure_referent_alive should be
entirely #if INCLUDE_ALL_GCS conditionalized, as should be the call
sites.  (Conditionalizing the call sites could be done by providing
dummy non-ALL_GC definitions.)

------------------------------------------------------------------------------
src/share/vm/prims/unsafe.cpp 
 301 UNSAFE_ENTRY(jobject, Unsafe_GetObject(JNIEnv *env, jobject unsafe, jobject obj, jlong offset)) {
...
 312   if (is_java_lang_ref_Reference_access(p, offset)) {
 313     ensure_referent_alive(v);
 314   }

This version performs the is_jlr_Reference_access even if the
collector doesn't care.  The original version first tested the GC to
see if it is one that cares (e.g. G1), and only then does the
additional testing.  I think the original testing order should be
restored.

Similarly in Unsafe_GetObjectVolatile. 

Hm, it seems that what's happening here is that I'm disagreeing with
some of Mikael's suggestions and preferring something closer to the
original proposal.  Mikael and I should talk about this before giving
more conflicting suggestions.

------------------------------------------------------------------------------ 
src/share/vm/prims/unsafe.cpp 
 301 UNSAFE_ENTRY(jobject, Unsafe_GetObject(JNIEnv *env, jobject unsafe, jobject obj, jlong offset)) {
...
 316   return JNIHandles::make_local(env, v);

This version delays making a handle over the value until the end.  The
original version did this early, and did the (conditional) keep-alive
with the result in a handle.

I don't see anything in the conditional keep-alive code that could
invalidate a naked oop, but it would be safer and more maintenance
proof to eagerly handlize as in the original version.

Similarly in Unsafe_GetObjectVolatile. 

------------------------------------------------------------------------------
src/share/vm/prims/unsafe.cpp 
 279 static bool is_java_lang_ref_Reference_access(oop o, jlong offset) {
 280   if (offset == java_lang_ref_Reference::referent_offset && o != NULL) {

The null test is superfluous, since we've already accessed a field
from it.  (Though the compiler should be able to optimize it away.)

------------------------------------------------------------------------------