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

[Kim Barrett]

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