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

[Richard Reingruber]

On Thu, 30 Mar 2023 15:33:34 GMT, Erik Österlund <eosterlund at openjdk.org> wrote:

> I'm okay with the general approach. I'm a bit worried about it though.

I'm also not perfectly comfortable with it. Now even more so.

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

We might crash intermittently but not in obscure ways. We would hit the guarantee I left in CompiledMethod::cleanup_inline_caches_impl(). It would be clear that the statically bound call lacks the dependency to the target holder. IMHO `check_path_to_callee()` would be cheap enough to have it in release builds at least when resolving an optimized call to an interpreted callee (I'd consider static calls to be always reachable through CLD handles). This would make the crashes less intermittent.
It would be obscure though why the dependency is missing. Probably you meant that.

I agree it's really hard to reason about the paths in the compilers when they emit optimized virtual calls. I don't want to do it. I though I could establish the requirement that the call target dependency must exist and that it is easy to discover. It would be the responsibility of compiler providers to meet the requirement. But I understand now that this is not robust enough.

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

Ok, so the relocation for optimized virtual calls should point to an oop slot, right?

> We already have support for calling register_nmethod multiple times due to C1 mirror patching that does a similar trick.

I see. This is needed if, e.g., the patched oop is in the young region.

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

So right now I don't feel like integrating the pr as it is.
I thought the approach was ok. In tests the call target holder is always reachable. I assumed we could even add it to CLD::_handles for the sake of robustness if the path is not found. It might look like a hack but given that in reality it is never done it still appears acceptable to me.

Anyways, thanks a lot for bearing that long with me.
Probably you're right. Every optimized call should refer to an oop slot for referencing the target.

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

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