RFR: 8340141: C1: rework ciMethod::equals following 8338471 [v3]

[Dean Long]

On Thu, 10 Oct 2024 22:53:03 GMT, Vladimir Ivanov <vlivanov at openjdk.org> wrote:

>> Dean Long has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   make sure to be in VM state when checking is_old
>
> src/hotspot/share/ci/ciMethod.cpp line 695:
> 
>> 693:   // Redefinition support.
>> 694:   if (this->is_old() || root_m->is_old()) {
>> 695:     return nullptr;
> 
> IMO you can safely drop this particular check. The one after `Dependencies::find_unique_concrete_method()` should be enough to preserve the invariant (`target == cha_monomorphic_target`) .
> 
> But thinking more about it, now I'm curious what happens when an old method is actually encountered. The fix conservatively rejects possible inlining opportunity, but it seems it doesn't invalidate resulting nmethod anymore. So, no recompilation attempt follows to recuperate that.
> 
> We could either record a evol dependency on a stale `Method` (to fail during nmethod installation step) or fail-fast the compilation (probably, implies additional checks to propagate the failure status).

Even if we check for stale Methods in various places, including invoke(), there is nothing to prevent the method from going stale after the last spot-check.  My understanding was that we already handle stale metadata as a precondition to creating the nmethod.  If we have a loophole there that lets stale metadata get through, then that's a separate existing bug for C1 and C2.
I was tempted to add a bailout, but the reason would be as a performance improvement to short-circuit wasted work, not to correct a stale metadata problem.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/21148#discussion_r1796199310