RFR: 8296440: Remove Method* handling from cleanup_inline_caches_impl [v5]

[Erik Österlund]

On Thu, 30 Mar 2023 14:42:32 GMT, Richard Reingruber <rrich at openjdk.org> wrote:

> So from my point of view this is ready for integration. Thanks for the help, feeback, and reviews!
> 
> Before I'd like to share some final thoughts. So the search in `check_path_to_callee()` is shallow. It might yield false positives. I hope for support by compiler folks adding dependencies at compile time, even if only needed to avoid false positives, and even more so, if no path exists.
> 
> To make this really robust, new dependencies could be added to the caller nmethod if no path to the holder of the call target is found. This is not possible atm but maybe the nmethod's oop pool could be changed to be a growable kind of (weak) OopStorage outside the code cache if needed.
> 
> An adhoc fix in the case of missing(?) path could be not to resolve the call if the callee should be interpreted. Instead the callee could be compiled in the background. Execution would continue without resolving the call.
> 
> @fisk may I ask if you are also ok with the general approach of this pr?

I'm okay with the general approach. I'm a bit worried about it though. With this approach, correct unloading of statically bound callsites relies on the dependency data used for deopt due to class loading, also implicitly embed enough information to deal with the unloading. Is there always a match? In order to answer that question we have to either walking all combinations of paths in which our compilers can generate a monomorphic callsite and see that it's correct, hope it doesn't break over time, and hope that the assert catches when it does, or we will end up crashing very intermittently in very obscure ways.
I have spent a fair amount of time looking through the different paths in which you get to the decision point of generating a statically bound callsite, and I *think* that it is currently correct. But there are so many paths, I find it hard to follow them all.
I would have preferred separating implicit dependency on all the reasons why a callsite became statically bound, with how to deal with correct unloading, so that it's easier to reason about it with certainty. For example, making sure there is an oop slot for each statically bound callsite, and when resolving the callsite, just ensure the holder oop is in there and register_nmethod() to the GC. We already have support for calling register_nmethod multiple times due to C1 mirror patching that does a similar trick. Then we could algorithmically ensure that the logic works as expected, regardless of why the callsite became statically bound.
Having said that, I'm okay with this approach too. We will probably run into the assert if something fishy happens, eventually. So I'll leave it to you to decide, and I'm okay with integrating as is.

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

PR Comment: https://git.openjdk.org/jdk/pull/12802#issuecomment-1490512796