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

Kim Barrett kim.barrett at oracle.com
Thu Sep 15 15:30:53 UTC 2016

> 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 (UseG1GC &&
      v != NULL &&
      is_java_lang_ref_Reference_access(o, offset)) {

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