RFR: 8268364: jmethod clearing should be done during unloading

[Coleen Phillimore]

On Thu, 1 Jul 2021 14:53:24 GMT, Daniel D. Daugherty <dcubed at openjdk.org> wrote:

>> This patch moves the jmethod clearing to ClassLoaderData::unload() but also adds a check to Method::checked_resolved_jmethod_id() to handle the case where ZGC may be unloading a class but not have gotten to ClassLoaderData::unload() yet.  JVMTI will read a NULL method for checked_resolved_jmethod_id() in this case, and not get a Method that will shortly, or has already been reclaimed in the Metaspace destructor.
>> Since I was there, I also added Method::is_valid_method() check to checked_resolve_jmethod_id. I don't think it's expensive anymore but it could be added under DEBUG.  Either way method->method_holder()->is_loader_alive() will crash if !is_valid_method so we should leave it.   As I wrote in the related issues, the bogus Method may have been because of a previous set of bugs with post_compiled_method_load events.
>> 
>> Tested with tiers 1-6 on linux-x64-debug and 1-3 on windows-x64-debug.
>> 
>> Also ran vmTestbase/nsk/{jdi,jvmti} tests with VM_OPTIONS=-XX:+UseZGC  -XX:ZCollectionInterval=0.01 -XX:ZFragmen
>> tationLimit=0
>
> src/hotspot/share/oops/method.cpp line 2262:
> 
>> 2260:   // Method should otherwise be valid. Assert for testing.
>> 2261:   assert(is_valid_method(o), "should be valid jmethodid");
>> 2262:   return o->method_holder()->is_loader_alive() ? o : NULL;
> 
> Let me ask the question a different way. Here's the two lines of code:
> 
> assert(is_valid_method(o), "should be valid jmethodid");
> return o->method_holder()->is_loader_alive() ? o : NULL;
> 
> 
> If `is_loader_alive()` can return false at this point, then what does
> the previous `is_valid_method(o)` call return in that case? Should the
> `assert()` call only be made in the case where `is_loader_alive()` returns
> true?
> 
> Based on what you said in your reply, the normal expectation is that the
> `assert()` call will pass because at that point we must have a valid Method.
> (I'm ignoring corruption here). If we have a valid Method, then that implies
> that the `is_loader_alive()` call must also return true. So if we're expecting
> the Method to be valid at the point of the `assert()` call, then why are we
> checking `is_loader_alive()`? It could be for robustness, but in that case,
> I would add a comment making that clear.

The assert is to check for corruptness but even if the loader isn't alive, the Method* should be valid.  For concurrent unloading (ie. zgc), checking that the method loader is alive goes like this:
method->method_holder() points to the method's InstanceKlass but the ->is_loader_alive will go to the ClassLoaderData and do a weak load of the holder oop (class_loader or mirror oop).  If the weak load fails, the loader is no longer alive, but the concurrent collector hasn't marked it as unloaded yet, but it will.  After the safepoint, the code is then safe to purge and that will reclaim memory for the Method and it's holder.

// Unloading support
bool ClassLoaderData::is_alive() const {
  bool alive = keep_alive()         // null class loader and incomplete non-strong hidden class.
      || (_holder.peek() != NULL);  // and not cleaned by the GC weak handle processing.

  return alive;
}

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

PR: https://git.openjdk.java.net/jdk/pull/4643