RFR: 8268364: jmethod clearing should be done during unloading

[Daniel D.Daugherty]

On Wed, 30 Jun 2021 22:48:03 GMT, Coleen Phillimore <coleenp 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

Still just comments because I'm not clear on the one code path.

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.

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

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