`invoke`d calls: record invoke signature in backedges #46010

timholy · 2022-07-12T20:34:33Z

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 · 2022-07-12T21:14:31Z

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.

aviatesk · 2022-07-13T09:17:16Z

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

base/compiler/optimize.jl

…44224) fix #44174.

timholy · 2022-07-13T21:23:14Z

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 f591258). Assuming CI passes, this seems ready for review ❤️ or a merge.

timholy · 2022-07-14T11:44:40Z

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

timholy · 2022-07-19T10:19:47Z

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 · 2022-07-21T18:01:17Z

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 · 2022-07-22T11:23:29Z

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

timholy · 2022-07-22T13:02:33Z

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

base/compiler/abstractinterpretation.jl

vtjnash

We also should teach jl_method_table_insert, in its direct call to invalidate_backedges, to ignore edges that aren't changed, due to the new invoke info

base/compiler/abstractinterpretation.jl

base/compiler/utilities.jl

src/gf.c

src/method.c

vtjnash · 2022-08-02T18:57:52Z

src/dump.c

+                        k = 0;
+                        while (k < nlive) {
+                            k = get_next_backedge(milive->backedges, k, &invokeTypes2, &belive2);
+                            if (belive == belive2 && invokeTypes == invokeTypes2) {


vtjnash · 2022-08-02T19:06:42Z

src/dump.c

@@ -2470,7 +2543,7 @@ static void jl_insert_backedges(jl_array_t *list, jl_array_t *targets)
                int32_t idx = idxs[j];
                jl_value_t *callee = jl_array_ptr_ref(targets, idx * 2);
                if (jl_is_method_instance(callee)) {
-                    jl_method_instance_add_backedge((jl_method_instance_t*)callee, caller);
+                    jl_method_instance_add_backedge((jl_method_instance_t*)callee, NULL, caller);


This should also need the invoke-types for the backedge too (sometimes)?

Ah, I guess we need to expand ~~targets~~list to include the invokesig?

src/dump.c

This allows a MethodInstance to store dispatch tuples as well as other MethodInstances among their backedges. The motivation is to properly handle invalidation with `invoke`, which allows calling a less-specific method and thus runs afoul of our standard mechanisms to detect "outdated" dispatch. Additionally: - provide backedge-iterators for both C and Julia that abstracts the specific storage mechanism. - fix a bug in the CodeInstance `relocatability` field, where methods that only return a constant (and hence store `nothing` for `inferred`) were deemed non-relocatable. - fix a bug in which #43990 should have checked that the method had not been deleted. Tests passed formerly simply because we weren't caching external CodeInstances that inferred down to a `Const`; fixing that exposed the bug. This bug has been exposed since merging #43990 for non-`Const` inference, and would affect Revise etc.

This mimics an `invoke` call

Co-authored-by: Jameson Nash <vtjnash@gmail.com>

Encode the `invokesig` in `ext_targets` so it gets cached to disk.

timholy · 2022-08-24T17:30:53Z

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.

This is a left-over piece of #46010, triggered by const args.

This is a left-over piece of #46010, triggered by const args. Fixes #44320

This updates to the logging format used by the most recent nightly build of Julia (1.9). While there has been a lot of churn in this area, there are reasons to hope that we're entering a period of stability: there are no known "holes" in our coverage after resolving the `invoke` issues (JuliaLang/julia#46010 and fixup PRs that came later), and the subsequent major rewrite of the invalidation logic (JuliaLang/julia#46920) suggests this format may have some durability.

This updates to the logging format used by the most recent nightly build of Julia (1.9). While there has been a lot of churn in this area, there are reasons to hope that we're entering a period of stability: there are no known "holes" in our coverage after resolving the `invoke` issues (JuliaLang/julia#46010 and fixup PRs that came later), and the subsequent major rewrite of the invalidation logic (JuliaLang/julia#46920) suggests this format may have some durability. Co-authored-by: Rik Huijzer <rikhuijzer@pm.me>

This updates to the logging format used by the most recent nightly build of Julia (1.9). While there has been a lot of churn in this area, there are reasons to hope that we're entering a period of stability: there are no known "holes" in our coverage after resolving the `invoke` issues (JuliaLang/julia#46010 and fixup PRs that came later), and the subsequent major rewrite of the invalidation logic during deserialization (JuliaLang/julia#46920) suggests this format may have some durability. Co-authored-by: Rik Huijzer <rikhuijzer@pm.me>

This includes only the changes to `dump.c` for this change, but excludes the functional part of the change (except for the additional bugfixes mentioned below). ORIGINAL COMMIT TEXT: This fixes a long-standing issue with how we've handled `invoke` calls with respect to method invalidation. When we load a package, we need to ask whether a given MethodInstance would be compiled in the same way now (aka, in the user's running session) as when the package was precompiled; in practice, the way we do that is to test whether the dispatches would be to the same methods in the current world-age. `invoke` presents special challenges because it allows the coder to deliberately select a different method than the one that would be chosen by ordinary dispatch; if there is no record of how this choice was made, it can look like it resolves to the wrong method and this can trigger invalidation. This allows a MethodInstance to store dispatch tuples as well as other MethodInstances among their backedges. Additionally: - provide backedge-iterators for both C and Julia that abstracts the specific storage mechanism. - fix a bug in the CodeInstance `relocatability` field, where methods that only return a constant (and hence store `nothing` for `inferred`) were deemed non-relocatable. - fix a bug in which #43990 should have checked that the method had not been deleted. Tests passed formerly simply because we weren't caching external CodeInstances that inferred down to a `Const`; fixing that exposed the bug. This bug has been exposed since merging #43990 for non-`Const` inference, and would affect Revise etc. Co-authored-by: Jameson Nash <vtjnash@gmail.com> (cherry picked from commits dd375e1, cb0721b, 9e39fe9)

timholy added backport 1.8 Change should be backported to release-1.8 compiler:latency Compiler latency compiler:precompilation Precompilation of modules labels Jul 12, 2022

oxinabox reviewed Jul 13, 2022

View reviewed changes

base/compiler/optimize.jl Show resolved Hide resolved

timholy referenced this pull request Jul 13, 2022

AbstractInterpreter: make it easier to overload pure/concrete-eval (#…

daa0849

…44224) fix #44174.

KristofferC mentioned this pull request Jul 17, 2022

release-1.8: Backports for 1.8-rc4 #46075

Merged

32 tasks

timholy force-pushed the teh/invokedTypes branch from e8cf7a9 to 9274f87 Compare July 21, 2022 16:11

timholy mentioned this pull request Jul 21, 2022

Avoid unnecessary invalidations for invoked fallbacks #45001

Closed

vtjnash reviewed Aug 2, 2022

View reviewed changes

base/compiler/abstractinterpretation.jl Outdated Show resolved Hide resolved

vtjnash reviewed Aug 2, 2022

View reviewed changes

src/dump.c Show resolved Hide resolved

KristofferC mentioned this pull request Aug 17, 2022

release-1.8: Backports for Julia 1.8.1 #46376

Merged

35 tasks

vchuravy removed the backport 1.8 Change should be backported to release-1.8 label Aug 20, 2022

timholy and others added 8 commits August 22, 2022 14:55

don't include subtype

7d5f1a8

Support precompile(::Method, argtypes)

e937235

This mimics an `invoke` call

fix an ambiguity

737efc8

Avoid name lookup

5808545

Fix sparams generation

fa5c237

Apply suggestions from code review

1b9535b

Co-authored-by: Jameson Nash <vtjnash@gmail.com>

Address review comments

d1923b0

timholy force-pushed the teh/invokedTypes branch from e620c90 to c9c87c1 Compare August 22, 2022 20:19

Cache invokesig in backedge data

5aaf76d

Encode the `invokesig` in `ext_targets` so it gets cached to disk.

timholy force-pushed the teh/invokedTypes branch from c9c87c1 to 5aaf76d Compare August 23, 2022 06:37

timholy added 2 commits August 24, 2022 11:42

Fix jl_egal call

945bcfc

Teach jl_method_table_insert about invoke backedges

8a9a87c

timholy added the status:merge me PR is reviewed. Merge when all tests are passing label Aug 24, 2022

timholy merged commit dd375e1 into master Aug 24, 2022

timholy deleted the teh/invokedTypes branch August 24, 2022 19:31

timholy removed the status:merge me PR is reviewed. Merge when all tests are passing label Aug 24, 2022

Keno mentioned this pull request Aug 27, 2022

Assertion failure when redefining invoke'd method #46503

Closed

timholy mentioned this pull request Aug 31, 2022

Disambiguate invalidation types in jl_insert_method_instance #46569

Merged

aviatesk mentioned this pull request Sep 12, 2022

inference: regression in BaseBenchmarks.InferenceBenchmarks.many_local_vars benchmark from #46010 #46713

Closed

timholy added a commit that referenced this pull request Sep 12, 2022

Support invoke backedges for const (fix #44320)

ddf6357

This is a left-over piece of #46010, triggered by const args.

timholy mentioned this pull request Sep 12, 2022

Support invoke backedges for const (fix #44320) #46715

Merged

timholy added a commit that referenced this pull request Sep 12, 2022

Support invoke backedges for const (#46715)

ad19f2f

This is a left-over piece of #46010, triggered by const args. Fixes #44320

timholy mentioned this pull request Oct 15, 2022

Update invalidation logging format timholy/SnoopCompile.jl#306

Merged

timholy mentioned this pull request Feb 6, 2023

longer compilation time in GMT.jl #44991

Closed

timholy mentioned this pull request Mar 10, 2023

gf: accelerate adding backedges back to old performance #48966

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`invoke`d calls: record invoke signature in backedges #46010

`invoke`d calls: record invoke signature in backedges #46010

timholy commented Jul 12, 2022 •

edited

Loading

timholy commented Jul 12, 2022

aviatesk commented Jul 13, 2022 •

edited

Loading

timholy commented Jul 13, 2022

timholy commented Jul 14, 2022

timholy commented Jul 19, 2022 •

edited

Loading

timholy commented Jul 21, 2022 •

edited

Loading

timholy commented Jul 22, 2022

timholy commented Jul 22, 2022 •

edited

Loading

vtjnash left a comment

vtjnash Aug 2, 2022

vtjnash Aug 2, 2022

timholy Aug 2, 2022 •

edited

Loading

timholy commented Aug 24, 2022 •

edited

Loading

invoked calls: record invoke signature in backedges #46010

invoked calls: record invoke signature in backedges #46010

Conversation

timholy commented Jul 12, 2022 • edited Loading

timholy commented Jul 12, 2022

aviatesk commented Jul 13, 2022 • edited Loading

timholy commented Jul 13, 2022

timholy commented Jul 14, 2022

timholy commented Jul 19, 2022 • edited Loading

timholy commented Jul 21, 2022 • edited Loading

timholy commented Jul 22, 2022

timholy commented Jul 22, 2022 • edited Loading

Load time

TTFX

vtjnash left a comment

Choose a reason for hiding this comment

vtjnash Aug 2, 2022

Choose a reason for hiding this comment

vtjnash Aug 2, 2022

Choose a reason for hiding this comment

timholy Aug 2, 2022 • edited Loading

Choose a reason for hiding this comment

timholy commented Aug 24, 2022 • edited Loading

`invoke`d calls: record invoke signature in backedges #46010

`invoke`d calls: record invoke signature in backedges #46010

timholy commented Jul 12, 2022 •

edited

Loading

aviatesk commented Jul 13, 2022 •

edited

Loading

timholy commented Jul 19, 2022 •

edited

Loading

timholy commented Jul 21, 2022 •

edited

Loading

timholy commented Jul 22, 2022 •

edited

Loading

timholy Aug 2, 2022 •

edited

Loading

timholy commented Aug 24, 2022 •

edited

Loading