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

[Vladimir Ivanov]

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

>>> 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.
>
> C1 does call 
> 
> 2159            dependency_recorder()->assert_evol_method(inline_target);
> 
> which in the CHA case would be `cha_monomorphic_target`, not `target`, so it looks like we may not detect if `target` is stale as long as `cha_monomorphic_target` is not.  It seems like a minor loophole, but I'm not sure what kind of problems it could cause, especially if the bytecodes of `target` are not used.

The scenario which concerns me is performance-related. If CHA conservatively disables inlining when concurrent class redefinition takes place during parsing, then there is no mechanism in place to recuperate possible loss of performance. Normally, if inlined method is redefined later during compilation, nmethod installation fails during dependency validation. But here no inlining happens (missed optimization opportunity) and call sites in generated code are resolved based on symbolic information (except rare cases when resolved method is attached to the call site, see `SharedRuntime::find_callee_info_helper()` for details), so there are no guarantees the stale `Method*` is recorded.

I agree that the window for such sequence of events is narrow, but it may be a source of surprising performance anomalies in rare cases.

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

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