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

[Dean Long]

On Thu, 10 Oct 2024 23:05:18 GMT, Dean Long <dlong at openjdk.org> wrote:

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

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

If I do that, then I can also revert the VM state changes.  However, I wasn't able to convince myself that this check is not needed.  If we end up returning root_m here as cha_monomorphic_target, it seems possible that it could be a new version of the method, and then target == cha_monomorphic_target would fail.

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

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