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

Kim Barrett kim.barrett at oracle.com
Wed Nov 1 17:57:10 UTC 2017


> On Nov 1, 2017, at 5:44 AM, Erik Österlund <erik.osterlund at oracle.com> wrote:
> 
> Hi Kim,
> 
> On 2017-11-01 06:03, Kim Barrett wrote:
>>> On Oct 30, 2017, at 7:14 AM, Doug Simon <doug.simon at oracle.com> wrote:
>>> 
>>> Hi Kim,
>>> 
>>> Thanks for the detailed review.
>>> 
>>>> On 29 Oct 2017, at 23:29, Kim Barrett <kim.barrett at oracle.com> wrote:
>>>> 
>>>> [added hotspot-gc-dev to cc list]
>>>> 
>>>>> On Oct 27, 2017, at 5:05 PM, Doug Simon <doug.simon at oracle.com> 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.



More information about the hotspot-gc-dev mailing list