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

Per Liden per.liden at oracle.com
Thu Sep 15 19:13:47 UTC 2016


Hi Martin,

On 2016-09-15 18:09, Doerr, Martin wrote:
> Hi Kim and Mikael,
>
> here's how it looks like according to the current state of the discussion:
> http://cr.openjdk.java.net/~mdoerr/8165489_G1_Unsafe/webrev.02/
>
> Thanks for your input. I think it looks good this way.
>
> If I'm not missing anything, the only open question is if v should be put into a handle which gets passed to ensure_satb_referent_alive. IMHO it looks better as it is now, but if you come to the conclusion that the handle is desired, I'll provide an update.

Looks good to me.

cheers,
Per

>
> Best regards,
> Martin
>
>
> -----Original Message-----
> From: Kim Barrett [mailto:kim.barrett at oracle.com]
> Sent: Donnerstag, 15. September 2016 17:31
> To: Mikael Gerdin <mikael.gerdin at oracle.com>
> Cc: Doerr, Martin <martin.doerr at sap.com>; hotspot-runtime-dev at openjdk.java.net; hotspot-gc-dev at openjdk.java.net
> Subject: Re: RFR(S): 8165489: Missing G1 barrier in Unsafe_GetObjectVolatile
>
>> On Sep 15, 2016, at 9:52 AM, Mikael Gerdin <mikael.gerdin at oracle.com> wrote:
>> 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/
>>> [...]
>>> 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.
>
> How about something like
>
> static void ensure_satb_referent_alive(oop o, jlong offset, oop v) {
> #if INCLUDE_ALL_GCS
>   if (UseG1GC &&
>       v != NULL &&
>       is_java_lang_ref_Reference_access(o, offset)) {
>     G1SATBCardTableModRefBS::enqueue(v);
>   }
> #endif
> }
>
> Note: is_jlr_Reference_access will need to be #if INCLUDE_ALL_GCS too,
> since otherwise gcc (at least, maybe others) may whine about a static function
> with no callers. Or make it inline rather than static.
>
>>> ------------------------------------------------------------------------------
>>> 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.
>
> I have no opinion regarding the different handle types here.  Early handlizing
> seemed more robust to me, but I don't feel strongly about it here.  It probably
> simplifies ensure code to not need to worry about handles.
>
>
>
>


More information about the hotspot-runtime-dev mailing list