RFR: 8322630: Remove ICStubs and related safepoints [v9]

[Erik Österlund]

On Sat, 10 Feb 2024 00:35:47 GMT, Dean Long <dlong at openjdk.org> wrote:

> If I just look at the new code, everything looks very reasonable. I tried to compare the new code to the old code, but quickly gave up.

:)

> Could you explain why opt_virtual can now be a direct call and CompiledIC is now only for virtual? It seems like we could have done that even with the old code.

Sure! So I think at some point we had static calls and virtual calls, with virtual calls using CompiledIC and static calls using CompiledStaticCall. I think there must have been some confusion at some point when introducing opt_virtual. They are virtual calls, so it might have seem "natural" to jam them into CompiledIC, and have CompiledIC deal with them. But since they in their code shapes are a lot more similar to static calls, with a direct call and a stub for interpreter entry, it never was a great fit. To deal with it we had to 1) ensure the data is always null as there is no data, because it isn't really an inline cache, 2) check for it everywhere to not spin up ICStubs when transitioning, because it isn't really an inline cache, and then when manipulating the callsite, have some some native call wrapper abstraction with virtual calls that would convert requests to update the inline cache to update the corresponding direct call and stub instead for optimized virtual calls, and 
 get out of the inline cache world.

To me this always seemed a bit backwards. So in my new implementation I instead accept that opt_virtual calls really are more like the static calls (generated code is identical), and have pretty much nothing to do with inline caches, despite being virtual calls, and made them both use a common DirectCall abstraction instead, that fit both of them, as they are both direct calls.

Could we have cleaned that up earlier? Yes, probably. But it was pretty ingrained and "interesting" to change with incremental changes. I figured this was my chance to do this right since I'm rewriting the CompiledIC file pretty much from scratch as you noticed. I hope you agree with this decision.

> Also, why don't we have to check for method->is_old() anymore?

The checks for is_old were there because when performing an inline cache transition, we had situations where we would need an ICStub, and after running out of ICStubs we would have to request a safepoint to refill ICStubs. That safepoint could sneak in a class redefinition operation, rendering the methods invalid, or at least is_old(). Since I removed ICStubs and don't need any ICStubs to transition inline caches, we also get any safepoints in this code. And therefore we can't get any class redefinition, and hence can remove all the is_old() checks, which are now effectively dead code.

Thanks for the review @dean-long! I updated the comments as requested.

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

PR Comment: https://git.openjdk.org/jdk/pull/17495#issuecomment-1938453986
PR Comment: https://git.openjdk.org/jdk/pull/17495#issuecomment-1938457516