Re: RFR: 8188102: [JVMCI] Convert special JVMCI oops in nmethod to jweak values

classic Classic list List threaded Threaded
7 messages Options
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8188102: [JVMCI] Convert special JVMCI oops in nmethod to jweak values

Vladimir Kozlov
CCing to GC group too.

Would be nice to run Hotspot testing with Graal as JIT. Katya, can you help with it?

Thanks,
Vladimir

On 10/27/17 2:05 PM, Doug Simon wrote:
> Please review this change that converts the JVMCI-specific object references in nmethod from oops to weak values. This removes GC API extensions added purely for these fields (e.g. so that G1 can insert it into the right remembered set, and when unloading an nmethod, to go and remove the nmethod from that remembered set).
>
> Testing: I've run the Graal unit tests (mx unittest --verbose --gc-after-test -Xlog:class+unload=trace) which trigger a lot of nmethod unloading.
>
> https://bugs.openjdk.java.net/browse/JDK-8188102
> http://cr.openjdk.java.net/~dnsimon/8188102/
>
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8188102: [JVMCI] Convert special JVMCI oops in nmethod to jweak values

Kim Barrett
[added hotspot-gc-dev to cc list]

> On Oct 27, 2017, at 5:05 PM, Doug Simon <[hidden email]> wrote:
>
> Please review this change that converts the JVMCI-specific object references in nmethod from oops to weak values. This removes GC API extensions added purely for these fields (e.g. so that G1 can insert it into the right remembered set, and when unloading an nmethod, to go and remove the nmethod from that remembered set).
>
> Testing: I've run the Graal unit tests (mx unittest --verbose --gc-after-test -Xlog:class+unload=trace) which trigger a lot of nmethod unloading.
>
> https://bugs.openjdk.java.net/browse/JDK-8188102
> http://cr.openjdk.java.net/~dnsimon/8188102/

I didn't look at the .java, .py, or project files.

------------------------------------------------------------------------------
src/hotspot/share/jvmci/jvmciCompilerToVM.cpp
1061       nmethod* nm = cb->as_nmethod_or_null();

This appears to be dead code now.

------------------------------------------------------------------------------
src/hotspot/share/code/nmethod.cpp
1023   assert(Universe::heap()->is_gc_active(), "should only be called during gc");
...
1036     if (!Universe::heap()->is_gc_active() && cause != NULL)
1037       cause->klass()->print_on(&ls);

I was going to mention that lines 1036-1037 are missing braces around
the if-body.  However, those lines appear to be dead code, given the
assertion on line 1023.

------------------------------------------------------------------------------
src/hotspot/share/code/nmethod.cpp
1504 bool nmethod::do_unloading_jvmci(BoolObjectClosure* is_alive, bool unloading_occurred) {
...
1506     oop installed_code = JNIHandles::resolve(_jvmci_installed_code);

Resolving a weak reference can keep an otherwise dead referent alive.
See JDK-8188055 for a discussion of the corresponding problem for
j.l.r.Reference.

Right now, I think JNIHandles doesn't provide a (public) solution to
what I think is being attempted here that works for all collectors.
There is in-progress work toward a solution, but it's just that, "in
progress".

As a (possibly interim) solution, a function like the following might
be added to JNIHandles (put the definition near resolve_jweak).

bool JNIHandles::is_global_weak_cleared(jweak handle) {
  assert(is_jweak(handle), "not a weak handle");
  return guard_value<false>(jweak_ref(handle)) == NULL;
}

(That's completely untested, and I haven't thought carefully about the
name.  And should get input from other GC folks on how to deal with
this.)  I *think* do_unloading_jvmci then becomes something like the
following (again, completely untested)

bool nmethod::do_unloading_jvmci(BoolObjectClosure* is_alive, bool unloading_occurred) {
  if (_jvmci_installed_code != NULL) {
    if (JNIHandles::is_global_weak_cleared(_jvmci_installed_code)) {
      if (_jvmci_installed_code_triggers_unloading) {
        make_unloaded(is_alive, NULL);
        return true;
      } else {
        clear_jvmci_installed_code();
      }
    }
  }
  return false;
}

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

Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8188102: [JVMCI] Convert special JVMCI oops in nmethod to jweak values

Kim Barrett
> On Oct 30, 2017, at 7:14 AM, Doug Simon <[hidden email]> wrote:
>
> Hi Kim,
>
> Thanks for the detailed review.
>
>> On 29 Oct 2017, at 23:29, Kim Barrett <[hidden email]> wrote:
>>
>> [added hotspot-gc-dev to cc list]
>>
>>> On Oct 27, 2017, at 5:05 PM, Doug Simon <[hidden email]> wrote:
>>>
>>> Please review this change that converts the JVMCI-specific object references in nmethod from oops to weak values. This removes GC API extensions added purely for these fields (e.g. so that G1 can insert it into the right remembered set, and when unloading an nmethod, to go and remove the nmethod from that remembered set).
>>>
>>> Testing: I've run the Graal unit tests (mx unittest --verbose --gc-after-test -Xlog:class+unload=trace) which trigger a lot of nmethod unloading.
>>>
>>> https://bugs.openjdk.java.net/browse/JDK-8188102
>>> http://cr.openjdk.java.net/~dnsimon/8188102/
>>
>> I didn't look at the .java, .py, or project files.
>>
>> ------------------------------------------------------------------------------
>> src/hotspot/share/jvmci/jvmciCompilerToVM.cpp
>> 1061       nmethod* nm = cb->as_nmethod_or_null();
>>
>> This appears to be dead code now.
>
> Indeed.
>
>> ------------------------------------------------------------------------------
>> src/hotspot/share/code/nmethod.cpp
>> 1023   assert(Universe::heap()->is_gc_active(), "should only be called during gc");
>> ...
>> 1036     if (!Universe::heap()->is_gc_active() && cause != NULL)
>> 1037       cause->klass()->print_on(&ls);
>>
>> I was going to mention that lines 1036-1037 are missing braces around
>> the if-body.  However, those lines appear to be dead code, given the
>> assertion on line 1023.
>
> Good catch. That problem pre-dates this webrev but I will clean it up here.
>
>> ------------------------------------------------------------------------------
>> src/hotspot/share/code/nmethod.cpp
>> 1504 bool nmethod::do_unloading_jvmci(BoolObjectClosure* is_alive, bool unloading_occurred) {
>> ...
>> 1506     oop installed_code = JNIHandles::resolve(_jvmci_installed_code);
>>
>> Resolving a weak reference can keep an otherwise dead referent alive.
>> See JDK-8188055 for a discussion of the corresponding problem for
>> j.l.r.Reference.
>>
>> Right now, I think JNIHandles doesn't provide a (public) solution to
>> what I think is being attempted here that works for all collectors.
>> There is in-progress work toward a solution, but it's just that, "in
>> progress".
>>
>> As a (possibly interim) solution, a function like the following might
>> be added to JNIHandles (put the definition near resolve_jweak).
>>
>> bool JNIHandles::is_global_weak_cleared(jweak handle) {
>> assert(is_jweak(handle), "not a weak handle");
>> return guard_value<false>(jweak_ref(handle)) == NULL;
>> }
>
> Adding JNIHandles::is_global_weak_cleared makes sense. I've put it the public section near destroy_weak_global instead of the private section where resolve_jweak is declared.
>
>> (That's completely untested, and I haven't thought carefully about the
>> name.  And should get input from other GC folks on how to deal with
>> this.)  I *think* do_unloading_jvmci then becomes something like the
>> following (again, completely untested)
>>
>> bool nmethod::do_unloading_jvmci(BoolObjectClosure* is_alive, bool unloading_occurred) {
>> if (_jvmci_installed_code != NULL) {
>>   if (JNIHandles::is_global_weak_cleared(_jvmci_installed_code)) {
>>     if (_jvmci_installed_code_triggers_unloading) {
>>       make_unloaded(is_alive, NULL);
>>       return true;
>>     } else {
>>       clear_jvmci_installed_code();
>>     }
>>   }
>> }
>> return false;
>> }
>
> I think your change works but comes at the cost of potentially preventing nmethod unloading for 1 extra (full?) GC cycle. It assumes that jweak clearing occurs before nmethod scanning. Is that guaranteed? If not, then I think what we want is:
>
> bool nmethod::do_unloading_jvmci(BoolObjectClosure* is_alive, bool unloading_occurred) {
>  if (_jvmci_installed_code != NULL) {
>    bool cleared = JNIHandles::is_global_weak_cleared(_jvmci_installed_code);
>    if (_jvmci_installed_code_triggers_unloading) {
>      if (cleared) {
>        // jweak reference processing has already cleared the referent
>        make_unloaded(is_alive, NULL);
>        return true;
>      } else {
>        oop installed_code = JNIHandles::resolve(_jvmci_installed_code);
>        if (can_unload(is_alive, (oop*)&installed_code, unloading_occurred)) {
>          return true;
>        }
>      }
>    } else {
>      if (cleared || !is_alive->do_object_b(JNIHandles::resolve(_jvmci_installed_code))) {
>        clear_jvmci_installed_code();
>      }
>    }
>  }
>  return false;
> }
>
> I've created a new webrev at http://cr.openjdk.java.net/~dnsimon/8188102_2.
>
> -Doug

The old code was looking for objects that are no longer alive; that
can't be determined until after reference processing, as finalization
could change the answer.  jweak clearing used to happen as part of
reference processing, but there's been some recent refactoring.  I
don't think that was intended to change the order of jweak processing
wrto other things, but I haven't looked at the code to verify I'm not
overlooking something.

In addition, I think

     } else {
       oop installed_code = JNIHandles::resolve(_jvmci_installed_code);
       if (can_unload(is_alive, (oop*)&installed_code, unloading_occurred)) {
         return true;
       }

doesn't work for some collectors (like G1), since the resolve will
force installed_code to be live (if jweak processing were performed
after nmethod processing), so can_unload will always return false.

Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8188102: [JVMCI] Convert special JVMCI oops in nmethod to jweak values

Erik Osterlund
Hi Kim,

On 2017-11-01 06:03, Kim Barrett wrote:

>> On Oct 30, 2017, at 7:14 AM, Doug Simon <[hidden email]> wrote:
>>
>> Hi Kim,
>>
>> Thanks for the detailed review.
>>
>>> On 29 Oct 2017, at 23:29, Kim Barrett <[hidden email]> wrote:
>>>
>>> [added hotspot-gc-dev to cc list]
>>>
>>>> On Oct 27, 2017, at 5:05 PM, Doug Simon <[hidden email]> wrote:
>>>>
>>>> Please review this change that converts the JVMCI-specific object references in nmethod from oops to weak values. This removes GC API extensions added purely for these fields (e.g. so that G1 can insert it into the right remembered set, and when unloading an nmethod, to go and remove the nmethod from that remembered set).
>>>>
>>>> Testing: I've run the Graal unit tests (mx unittest --verbose --gc-after-test -Xlog:class+unload=trace) which trigger a lot of nmethod unloading.
>>>>
>>>> https://bugs.openjdk.java.net/browse/JDK-8188102
>>>> http://cr.openjdk.java.net/~dnsimon/8188102/
>>> I didn't look at the .java, .py, or project files.
>>>
>>> ------------------------------------------------------------------------------
>>> src/hotspot/share/jvmci/jvmciCompilerToVM.cpp
>>> 1061       nmethod* nm = cb->as_nmethod_or_null();
>>>
>>> This appears to be dead code now.
>> Indeed.
>>
>>> ------------------------------------------------------------------------------
>>> src/hotspot/share/code/nmethod.cpp
>>> 1023   assert(Universe::heap()->is_gc_active(), "should only be called during gc");
>>> ...
>>> 1036     if (!Universe::heap()->is_gc_active() && cause != NULL)
>>> 1037       cause->klass()->print_on(&ls);
>>>
>>> I was going to mention that lines 1036-1037 are missing braces around
>>> the if-body.  However, those lines appear to be dead code, given the
>>> assertion on line 1023.
>> Good catch. That problem pre-dates this webrev but I will clean it up here.
>>
>>> ------------------------------------------------------------------------------
>>> src/hotspot/share/code/nmethod.cpp
>>> 1504 bool nmethod::do_unloading_jvmci(BoolObjectClosure* is_alive, bool unloading_occurred) {
>>> ...
>>> 1506     oop installed_code = JNIHandles::resolve(_jvmci_installed_code);
>>>
>>> Resolving a weak reference can keep an otherwise dead referent alive.
>>> See JDK-8188055 for a discussion of the corresponding problem for
>>> j.l.r.Reference.
>>>
>>> Right now, I think JNIHandles doesn't provide a (public) solution to
>>> what I think is being attempted here that works for all collectors.
>>> There is in-progress work toward a solution, but it's just that, "in
>>> progress".
>>>
>>> As a (possibly interim) solution, a function like the following might
>>> be added to JNIHandles (put the definition near resolve_jweak).
>>>
>>> bool JNIHandles::is_global_weak_cleared(jweak handle) {
>>> assert(is_jweak(handle), "not a weak handle");
>>> return guard_value<false>(jweak_ref(handle)) == NULL;
>>> }
>> Adding JNIHandles::is_global_weak_cleared makes sense. I've put it the public section near destroy_weak_global instead of the private section where resolve_jweak is declared.
>>
>>> (That's completely untested, and I haven't thought carefully about the
>>> name.  And should get input from other GC folks on how to deal with
>>> this.)  I *think* do_unloading_jvmci then becomes something like the
>>> following (again, completely untested)
>>>
>>> bool nmethod::do_unloading_jvmci(BoolObjectClosure* is_alive, bool unloading_occurred) {
>>> if (_jvmci_installed_code != NULL) {
>>>    if (JNIHandles::is_global_weak_cleared(_jvmci_installed_code)) {
>>>      if (_jvmci_installed_code_triggers_unloading) {
>>>        make_unloaded(is_alive, NULL);
>>>        return true;
>>>      } else {
>>>        clear_jvmci_installed_code();
>>>      }
>>>    }
>>> }
>>> return false;
>>> }
>> I think your change works but comes at the cost of potentially preventing nmethod unloading for 1 extra (full?) GC cycle. It assumes that jweak clearing occurs before nmethod scanning. Is that guaranteed? If not, then I think what we want is:
>>
>> bool nmethod::do_unloading_jvmci(BoolObjectClosure* is_alive, bool unloading_occurred) {
>>   if (_jvmci_installed_code != NULL) {
>>     bool cleared = JNIHandles::is_global_weak_cleared(_jvmci_installed_code);
>>     if (_jvmci_installed_code_triggers_unloading) {
>>       if (cleared) {
>>         // jweak reference processing has already cleared the referent
>>         make_unloaded(is_alive, NULL);
>>         return true;
>>       } else {
>>         oop installed_code = JNIHandles::resolve(_jvmci_installed_code);
>>         if (can_unload(is_alive, (oop*)&installed_code, unloading_occurred)) {
>>           return true;
>>         }
>>       }
>>     } else {
>>       if (cleared || !is_alive->do_object_b(JNIHandles::resolve(_jvmci_installed_code))) {
>>         clear_jvmci_installed_code();
>>       }
>>     }
>>   }
>>   return false;
>> }
>>
>> I've created a new webrev at http://cr.openjdk.java.net/~dnsimon/8188102_2.
>>
>> -Doug
> The old code was looking for objects that are no longer alive; that
> can't be determined until after reference processing, as finalization
> could change the answer.  jweak clearing used to happen as part of
> reference processing, but there's been some recent refactoring.  I
> don't think that was intended to change the order of jweak processing
> wrto other things, but I haven't looked at the code to verify I'm not
> overlooking something.
>
> In addition, I think
>
>       } else {
>         oop installed_code = JNIHandles::resolve(_jvmci_installed_code);
>         if (can_unload(is_alive, (oop*)&installed_code, unloading_occurred)) {
>           return true;
>         }
>
> doesn't work for some collectors (like G1), since the resolve will
> force installed_code to be live (if jweak processing were performed
> after nmethod processing), so can_unload will always return false.
>

JNIHandles::resolve with G1 should conditionally enqueue the oops if the
SATB queue is activated, which it will not be after marking terminated.
At this point, marking should have terminated already, and hence the
resolve will not change the liveness of the oop. So I don't quite see
how the resolve will change the oop to live here. Am I missing something?

Having said that, I agree this should be a "peek" resolve, which we may
implement soon.

Thanks,
/Erik
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8188102: [JVMCI] Convert special JVMCI oops in nmethod to jweak values

Kim Barrett
> On Nov 1, 2017, at 5:44 AM, Erik Österlund <[hidden email]> wrote:
>
> Hi Kim,
>
> On 2017-11-01 06:03, Kim Barrett wrote:
>>> On Oct 30, 2017, at 7:14 AM, Doug Simon <[hidden email]> wrote:
>>>
>>> Hi Kim,
>>>
>>> Thanks for the detailed review.
>>>
>>>> On 29 Oct 2017, at 23:29, Kim Barrett <[hidden email]> wrote:
>>>>
>>>> [added hotspot-gc-dev to cc list]
>>>>
>>>>> On Oct 27, 2017, at 5:05 PM, Doug Simon <[hidden email]> wrote:
>>>>>
>>>>> Please review this change that converts the JVMCI-specific object references in nmethod from oops to weak values. This removes GC API extensions added purely for these fields (e.g. so that G1 can insert it into the right remembered set, and when unloading an nmethod, to go and remove the nmethod from that remembered set).
>>>>>
>>>>> Testing: I've run the Graal unit tests (mx unittest --verbose --gc-after-test -Xlog:class+unload=trace) which trigger a lot of nmethod unloading.
>>>>>
>>>>> https://bugs.openjdk.java.net/browse/JDK-8188102
>>>>> http://cr.openjdk.java.net/~dnsimon/8188102/
>>>> I didn't look at the .java, .py, or project files.
>>>>
>>>> ------------------------------------------------------------------------------
>>>> src/hotspot/share/jvmci/jvmciCompilerToVM.cpp
>>>> 1061       nmethod* nm = cb->as_nmethod_or_null();
>>>>
>>>> This appears to be dead code now.
>>> Indeed.
>>>
>>>> ------------------------------------------------------------------------------
>>>> src/hotspot/share/code/nmethod.cpp
>>>> 1023   assert(Universe::heap()->is_gc_active(), "should only be called during gc");
>>>> ...
>>>> 1036     if (!Universe::heap()->is_gc_active() && cause != NULL)
>>>> 1037       cause->klass()->print_on(&ls);
>>>>
>>>> I was going to mention that lines 1036-1037 are missing braces around
>>>> the if-body.  However, those lines appear to be dead code, given the
>>>> assertion on line 1023.
>>> Good catch. That problem pre-dates this webrev but I will clean it up here.
>>>
>>>> ------------------------------------------------------------------------------
>>>> src/hotspot/share/code/nmethod.cpp
>>>> 1504 bool nmethod::do_unloading_jvmci(BoolObjectClosure* is_alive, bool unloading_occurred) {
>>>> ...
>>>> 1506     oop installed_code = JNIHandles::resolve(_jvmci_installed_code);
>>>>
>>>> Resolving a weak reference can keep an otherwise dead referent alive.
>>>> See JDK-8188055 for a discussion of the corresponding problem for
>>>> j.l.r.Reference.
>>>>
>>>> Right now, I think JNIHandles doesn't provide a (public) solution to
>>>> what I think is being attempted here that works for all collectors.
>>>> There is in-progress work toward a solution, but it's just that, "in
>>>> progress".
>>>>
>>>> As a (possibly interim) solution, a function like the following might
>>>> be added to JNIHandles (put the definition near resolve_jweak).
>>>>
>>>> bool JNIHandles::is_global_weak_cleared(jweak handle) {
>>>> assert(is_jweak(handle), "not a weak handle");
>>>> return guard_value<false>(jweak_ref(handle)) == NULL;
>>>> }
>>> Adding JNIHandles::is_global_weak_cleared makes sense. I've put it the public section near destroy_weak_global instead of the private section where resolve_jweak is declared.
>>>
>>>> (That's completely untested, and I haven't thought carefully about the
>>>> name.  And should get input from other GC folks on how to deal with
>>>> this.)  I *think* do_unloading_jvmci then becomes something like the
>>>> following (again, completely untested)
>>>>
>>>> bool nmethod::do_unloading_jvmci(BoolObjectClosure* is_alive, bool unloading_occurred) {
>>>> if (_jvmci_installed_code != NULL) {
>>>>   if (JNIHandles::is_global_weak_cleared(_jvmci_installed_code)) {
>>>>     if (_jvmci_installed_code_triggers_unloading) {
>>>>       make_unloaded(is_alive, NULL);
>>>>       return true;
>>>>     } else {
>>>>       clear_jvmci_installed_code();
>>>>     }
>>>>   }
>>>> }
>>>> return false;
>>>> }
>>> I think your change works but comes at the cost of potentially preventing nmethod unloading for 1 extra (full?) GC cycle. It assumes that jweak clearing occurs before nmethod scanning. Is that guaranteed? If not, then I think what we want is:
>>>
>>> bool nmethod::do_unloading_jvmci(BoolObjectClosure* is_alive, bool unloading_occurred) {
>>>  if (_jvmci_installed_code != NULL) {
>>>    bool cleared = JNIHandles::is_global_weak_cleared(_jvmci_installed_code);
>>>    if (_jvmci_installed_code_triggers_unloading) {
>>>      if (cleared) {
>>>        // jweak reference processing has already cleared the referent
>>>        make_unloaded(is_alive, NULL);
>>>        return true;
>>>      } else {
>>>        oop installed_code = JNIHandles::resolve(_jvmci_installed_code);
>>>        if (can_unload(is_alive, (oop*)&installed_code, unloading_occurred)) {
>>>          return true;
>>>        }
>>>      }
>>>    } else {
>>>      if (cleared || !is_alive->do_object_b(JNIHandles::resolve(_jvmci_installed_code))) {
>>>        clear_jvmci_installed_code();
>>>      }
>>>    }
>>>  }
>>>  return false;
>>> }
>>>
>>> I've created a new webrev at http://cr.openjdk.java.net/~dnsimon/8188102_2.
>>>
>>> -Doug
>> The old code was looking for objects that are no longer alive; that
>> can't be determined until after reference processing, as finalization
>> could change the answer.  jweak clearing used to happen as part of
>> reference processing, but there's been some recent refactoring.  I
>> don't think that was intended to change the order of jweak processing
>> wrto other things, but I haven't looked at the code to verify I'm not
>> overlooking something.
>>
>> In addition, I think
>>
>>      } else {
>>        oop installed_code = JNIHandles::resolve(_jvmci_installed_code);
>>        if (can_unload(is_alive, (oop*)&installed_code, unloading_occurred)) {
>>          return true;
>>        }
>>
>> doesn't work for some collectors (like G1), since the resolve will
>> force installed_code to be live (if jweak processing were performed
>> after nmethod processing), so can_unload will always return false.
>>
>
> JNIHandles::resolve with G1 should conditionally enqueue the oops if the SATB queue is activated, which it will not be after marking terminated. At this point, marking should have terminated already, and hence the resolve will not change the liveness of the oop. So I don't quite see how the resolve will change the oop to live here. Am I missing something?

Does this nmethod pass get run during young collections?  If not, then maybe you are right that it doesn’t matter.

But I still think that a cleanup pass that is based on using weak references like this should just be properly ordered
wrto weak reference processing, as it presumably already was.  In which case the resolve and can_unload is
effectively useless; either the jweak has been cleared and we won’t reach this clause, or the jweak has not been
cleared because the referent is still live, in which case can_unload will always return false and there was no point
in calling it.

> Having said that, I agree this should be a "peek" resolve, which we may implement soon.

I think there shouldn’t be a resolve at all, “peek” or otherwise.

Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8188102: [JVMCI] Convert special JVMCI oops in nmethod to jweak values

Tom Rodriguez-2
Looks good to me.

tom

Doug Simon wrote:

>
>> On 2 Nov 2017, at 04:47, Kim Barrett<[hidden email]>  wrote:
>>
>>> On Nov 1, 2017, at 5:10 PM, Doug Simon<[hidden email]>  wrote:
>>>
>>>
>>>
>>>> On 1 Nov 2017, at 18:57, Kim Barrett<[hidden email]>  wrote:
>>>>
>>>>> On Nov 1, 2017, at 5:44 AM, Erik Österlund<[hidden email]>  wrote:
>>>>>
>>>>> Hi Kim,
>>>>>
>>>>> On 2017-11-01 06:03, Kim Barrett wrote:
>>>>>>> On Oct 30, 2017, at 7:14 AM, Doug Simon<[hidden email]>  wrote:
>>>>> JNIHandles::resolve with G1 should conditionally enqueue the oops if the SATB queue is activated, which it will not be after marking terminated. At this point, marking should have terminated already, and hence the resolve will not change the liveness of the oop. So I don't quite see how the resolve will change the oop to live here. Am I missing something?
>>>> Does this nmethod pass get run during young collections?  If not, then maybe you are right that it doesn’t matter.
>>>>
>>>> But I still think that a cleanup pass that is based on using weak references like this should just be properly ordered
>>>> wrto weak reference processing, as it presumably already was.  In which case the resolve and can_unload is
>>>> effectively useless; either the jweak has been cleared and we won’t reach this clause, or the jweak has not been
>>>> cleared because the referent is still live, in which case can_unload will always return false and there was no point
>>>> in calling it.
>>> I agree. As stated previously, my (defensive) code is based on assuming that nmethod unloading might happen before reference processing. If it never/rarely happens, then your suggested change is the right one. I did some extra testing and never saw it happen. Based on that, I'd like to adopt your solution. In the worst case, nmethod unloading will be delayed one full GC cycle if nmethod unloading precedes reference processing.
>>>
>>> I've created another rev based on this:
>>>
>>> http://cr.openjdk.java.net/~dnsimon/8188102_3
>>>
>>> -Doug
>> This looks good.
>>
>> ------------------------------------------------------------------------------
>> src/hotspot/share/runtime/jniHandles.cpp
>>
>> I'd like is_global_weak_cleared moved up a few lines, immediately
>> following the specializations of resolve_jweak, which I think it is
>> closely related to.
>>
>> I don't need another webrev for this.
>
> Will do. I updated the latest webrev in place.
>
>> ------------------------------------------------------------------------------
>> src/hotspot/share/code/nmethod.cpp
>>
>> 2934 char* nmethod::jvmci_installed_code_name(char* buf, size_t buflen) {
>> 2935   if (!this->is_compiled_by_jvmci()) {
>> 2936     return NULL;
>> 2937   }
>> 2938   oop installed_code = JNIHandles::resolve(_jvmci_installed_code);
>>
>> Are there any circumstances where it would be unfortunate to have
>> getting the name keep alive the object? I was thinking logging. Or
>> perhaps it's a feature that getting the name might artificially extend
>> the lifetime? Looking at the callers, it doesn't seem like a problem
>> right now.
>
> Right, this is only called from tracing or debugging code.
>
>> This is one of those places where a "peek" access (coming soon from
>> the GC interface) might be appropriate.
>
> This potential for resolving a jweak keeping the referent alive is unfortunate but I'm guessing it's also unavoidable.
>
> Thanks once again for a very helpful review!
>
> -Doug
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8188102: [JVMCI] Convert special JVMCI oops in nmethod to jweak values

Vladimir Kozlov
In reply to this post by Kim Barrett
Seems good to me.

Thanks,
Vladimir

On 11/2/17 12:37 AM, Doug Simon wrote:

>
>
>> On 2 Nov 2017, at 04:47, Kim Barrett <[hidden email]> wrote:
>>
>>> On Nov 1, 2017, at 5:10 PM, Doug Simon <[hidden email]> wrote:
>>>
>>>
>>>
>>>> On 1 Nov 2017, at 18:57, Kim Barrett <[hidden email]> wrote:
>>>>
>>>>> On Nov 1, 2017, at 5:44 AM, Erik Österlund <[hidden email]> wrote:
>>>>>
>>>>> Hi Kim,
>>>>>
>>>>> On 2017-11-01 06:03, Kim Barrett wrote:
>>>>>>> On Oct 30, 2017, at 7:14 AM, Doug Simon <[hidden email]> wrote:
>>>>> JNIHandles::resolve with G1 should conditionally enqueue the oops if the SATB queue is activated, which it will not be after marking terminated. At this point, marking should have terminated already, and hence the resolve will not change the liveness of the oop. So I don't quite see how the resolve will change the oop to live here. Am I missing something?
>>>>
>>>> Does this nmethod pass get run during young collections?  If not, then maybe you are right that it doesn’t matter.
>>>>
>>>> But I still think that a cleanup pass that is based on using weak references like this should just be properly ordered
>>>> wrto weak reference processing, as it presumably already was.  In which case the resolve and can_unload is
>>>> effectively useless; either the jweak has been cleared and we won’t reach this clause, or the jweak has not been
>>>> cleared because the referent is still live, in which case can_unload will always return false and there was no point
>>>> in calling it.
>>>
>>> I agree. As stated previously, my (defensive) code is based on assuming that nmethod unloading might happen before reference processing. If it never/rarely happens, then your suggested change is the right one. I did some extra testing and never saw it happen. Based on that, I'd like to adopt your solution. In the worst case, nmethod unloading will be delayed one full GC cycle if nmethod unloading precedes reference processing.
>>>
>>> I've created another rev based on this:
>>>
>>> http://cr.openjdk.java.net/~dnsimon/8188102_3
>>>
>>> -Doug
>>
>> This looks good.
>
> Can I please get sign off from someone in the compiler team. Thanks.
>
> -Doug
>