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

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

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

Doug Simon @ Oracle
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

Doug Simon @ Oracle
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

Reply | Threaded
Open this post in threaded view
|

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

Doug Simon @ Oracle


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

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

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

This is one of those places where a "peek" access (coming soon from
the GC interface) might be appropriate.

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


Reply | Threaded
Open this post in threaded view
|

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

Doug Simon @ Oracle


> 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

Doug Simon @ Oracle
In reply to this post by Kim Barrett


> 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