julia icon indicating copy to clipboard operation
julia copied to clipboard

`invoke`d calls: record invoke signature in backedges

Open timholy opened this issue 3 years ago • 8 comments

This PR fixes a corner case in our current scheme for invalidating outdated methods when loading .ji files. What we want to do is test whether a given MethodInstance would be compiled in the same way now as when the package was precompiled; for efficiency, what we actually do is test whether the dispatches would be to the same methods in the current world-age. It turns out that we've long had a "hole" in our assessment: for calls made by invoke,

f(arg::SpecificType) = invoke(f, Tuple{GenericType}, arg)

it looks like an outdated method got called because the signature is not the most specific one (f(::GenericType) gets compiled for specialization f(::SpecificType), which looks like a wrong or outdated choice of method), and we have no record that a less-specific method was deliberately chosen via invoke. Incorrectly classifying this backedge as outdated triggers unnecessary invalidation, which propagates all the way up call chains; since we happen to have an invoke in our broadcasting infrastructure, it means that in practice this issue is much more significant than the relatively modest number of invoke calls one finds in Base, because almost all code that uses broadcasting gets invalidated, and that's quite a lot of package code.

This works by expanding the kinds of backedges to 3:

  • (traditional) known dispatch backedges which record the caller::MethodInstance in (callee::MethodInstance).backedges
  • (new) known dispatch backedges from invoke which record the invokesig::Type, caller::MethodInstance in (callee::MethodInstance).backedges as a sequential pair
  • (traditional) for unknown dispatch, the MethodTable stores signature, caller pairs, where signature stores what is known by inference about the types of the arguments.

Thus the invoked backedges are intermingled with traditional ones in callee.backedges, but they have a format similar to what one finds in MethodTables.

By storing invokesig we get a second chance to check whether the method is the one that would currently be chosen by invokesig rather than the signature of the generated specialization of callee. Thus this should robustly fix the invalidation issue.

Closes #45001

timholy avatar Jul 12 '22 20:07 timholy

Out of curiosity, why do we add backedges from ssair/inlining.jl? They already seem to be added by ordinary inference, but for this PR I had to make sure they were added in the same way so that eventually they could be de-duplicated.

timholy avatar Jul 12 '22 21:07 timholy

#45934 introduced some conflicts on this PR. I will update this PR to resolve the conflicts.

aviatesk avatar Jul 13 '22 09:07 aviatesk

I think all the niggling issues have been fixed, and it even fixes a genuine invalidation bug that's been lurking in 1.8 (see f59125872deae7e5b53ba82d1f62ec839657a865). Assuming CI passes, this seems ready for review :heart: or a merge.

timholy avatar Jul 13 '22 21:07 timholy

Doctest failure should have been fixed in #46021. I think the rest are unrelated.

timholy avatar Jul 14 '22 11:07 timholy

Don't merge yet, we still need other changes. In particular, (1) we need precompile(mi::MethodInstance) and likely also precompile(m::Method, invokesig) because otherwise we can't force precompilation of an invoked method that wouldn't have been chosen by normal dispatch, and (2) I've found some packages where this PR is not yet sufficient to prevent invalidation (I suspect there are other cases in inference that need special handling). Will investigate shortly.

timholy avatar Jul 19 '22 10:07 timholy

I rebased & squashed, and now the second tiny commit fixes (2), i.e., this now seems to solve the use cases for which it was intended. The third implements precompile support for invoked signatures---not essential, but something that symmetry suggests we should have.

I think this is ready to merge, pending the results from CI. I would love a review but will merge soon in any case.

timholy avatar Jul 21 '22 18:07 timholy

As far as I can tell none of these failures are related.

timholy avatar Jul 22 '22 11:07 timholy

I've also run some benchmarks to see if there are any load-time regressions. This represents a pair of runs (shown is centroid ± half-width) on three different Julia versions. tl/dr: everything is fine, if anything this is an improvement.

Load time

Package 1.8rc3 master this PR
CSV 1.7±0.0 4.3±0.1 4.2±0.1
DataFrames 1.3±0.0 3.3±0.1 3.0±0.0
GLMakie 9.4±0.4 11.7±0.3 10.4±0.1
LV 2.8±0.1 3.9±0.0 3.9±0.2
OrdinaryDiffEq 6.2±0.3 9.1±0.4 8.5±0.2
ModelingToolkit 14.1±0.8 17.9±0.5 14.6±0.6
Flux 8.0±0.3 10.3±0.5 9.5±0.5
JuMP 3.1±0.0 5.5±0.1 5.0±0.2
ImageFiltering 1.9±0.0 4.0±0.0 3.7±0.1

There seem to be no regressions for this PR compared to master, and possibly a benefit. (It's not impossible because load time depends a lot on backedge insertion, and present/absence of methodinstances in the cache affects this.)

Sadly, at some point master got much slower at loading packages :cry:, but that has nothing to do with this PR. Identifying the cause and fixing it is work for another PR and day.

TTFX

Package 1.8rc3 master this PR
CSV 6.2±0.1 6.8±0.1 6.5±0.0
DataFrames 9.9±0.1 11.1±0.3 10.7±0.1
GLMakie 27.9±0.6 28.2±0.4 26.0±0.2
LV 8.3±0.0 8.1±0.0 8.0±0.1
OrdinaryDiffEq 1.9±0.2 1.7±0.1 1.6±0.1
ModelingToolkit 40.6±1.4 50.1±6.5 38.3±1.3
Flux 14.2±0.1 15.7±0.3 15.5±0.7
JuMP 4.9±0.1 5.9±0.1 5.7±0.0
ImageFiltering 1.4±0.1 1.4±0.0 1.4±0.0

Also a win here (not quite sure what happened in the case of ModelingToolkit on master). GLMakie is not quite in the realm of "tolerable" but we are getting there!

This is with a Startup package using SnoopPrecompile to ensure that all needed methods are cached.

timholy avatar Jul 22 '22 13:07 timholy

I think this can be merged if all tests pass (they do locally). This has become quite the intrusive change, and I no longer think this is appropriate for backporting, at least not until it's lived on master for quite some time (and most likely, not at all). I discussed this with @vchuravy and he kindly removed the backport-1.8 label. Just letting folks know this is aligned with my own views.

timholy avatar Aug 24 '22 17:08 timholy