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

[Mikael Gerdin]

Hi Kim

On 2016-09-13 19:54, Kim Barrett wrote:
>> 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.

My main gripe with the initial suggested change was the name of 
G1SATB_registerReference.
Since I couldn't come up with a name I was happy about which could 
communicate everything that the function does
register_java_lang_ref_Reference_referent_if_running_g1 I thought that 
splitting the function into two was a reasonable approach.

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

Creating the JNI handle at return is a very common pattern in the JNI 
entry points.

My personal opinion is that if we are going to move towards reducing the 
number of raw oops overall we should prefer to use VM handles instead of 
JNI handles and only use JNI handles for returning and accepting 
parameters from JNI code. Code to convert between the handle types could 
help with reducing the verbosity of code more rich in handles.

However, ff you really feel like keeping the handle creation before the 
enqueue check then I won't press my point any further.

/Mikael

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