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

Erik Österlund erik.osterlund at oracle.com
Wed Nov 1 09:44:53 UTC 2017


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?

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

Thanks,
/Erik



More information about the hotspot-gc-dev mailing list