Additional fixes for import_methods with JIT#9248
Conversation
|
@enebo If you get a change to review this after the fact, my only concern is if the |
Having a different call site but the same call site ID doesn't seem to make sense. Removing that additional clone parameter eliminates most clone constructors.
ca2e6a8 to
d474a55
Compare
|
@headius That value is used for 2 things: profiling and finding split point when an inline happens. clone()'ing an instr to a different version in theory is still the same location(callsite) so you could argue it should be the same. However, there are a few caveats:
I think though any place we cloneForInlining which is where most of this is occuring it fits into 1 above. The original problem basically is 2 above. The spirit of profiling is to find things which do not change. At worst, giving a new callsiteId if it was done in error would only slow down profiling. I think this looks good to me from what I see in the PR. |
|
@enebo ok that's the sort of thing I was thinking of. If we ever get back to speculative profiling and optimizing I'm sure we will revisit this anyway. |
Additional fixes on top of #9237.
That PR modified
import_modulelogic to partially clear optimized interpreter state from cloned methods, but did not do the same for JIT-compiled methods. This PR has the additional changes:CallSitein the primaryCallBaseconstructor. This avoids the extra logic to fix up non-refined call sites being passed in from cloned call instructions when a refined site is needed. It is better for us to have the call site and call site ID be new for every call instruction anyway.resetmethod toMixedModeIRMethodto also clear any JIT-compiled version of the method.This does not fix the larger problem of precompiled methods (AOT, at boot or at load, using
CompiledIRMethod) being unable to recompile after they have been cloned, but 99% of users will run in interpreted or JIT mode, not AOT.