Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avoid unnecessary invalidations for invoked fallbacks #45001

Closed
wants to merge 1 commit into from

Conversation

timholy
Copy link
Member

@timholy timholy commented Apr 16, 2022

While investigating a report in https://discourse.julialang.org/t/package-load-time-regressions-in-v1-8-beta3/78875
it became clear that some precompilation statements were not effective
due to invalidation. It turns out that these have the pattern

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

When f gets compiled for a new subtype of SpecificType, the
external cached MethodInstance gets invalidated during the check
for validity of new specializations of external methods. invoke
is a specific source of trouble because it can dispatch to a less
specific method, and the test for validity is essentially to check
whether the chosen method is still the one that would be called by
ordinary dispatch (i.e., a test of whether the precompiled method
has been superseded by a more specific one).

This is a hack that works around this issue for copyto!, exploiting
the fact that if one is calling into a closed module then all of the
methods were available when the precompile was generated and hence
there must have been a reason for the choice. But being
able to mark calls as having been issued from an invoke would
be a much better solution. Without that, I don't see a way for a
package-level invoke that calls a Base or Core.Compiler fallback
to work properly. (Cthulhu is an example.)

While investigating a report in https://discourse.julialang.org/t/package-load-time-regressions-in-v1-8-beta3/78875
it became clear that some precompilation statements were not effective
due to invalidation.  It turns out that these have the pattern

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

When `f` gets compiled for a new subtype of `SpecificType`, the
external cached MethodInstance gets invalidated because of the
reliance on a less specific method.

This is a hack that works around this issue for `copyto!`, but being
able to mark calls as having been issued from an `invoke` would
be a much better solution.
@timholy timholy added compiler:latency Compiler latency backport 1.8 Change should be backported to release-1.8 labels Apr 16, 2022
@aviatesk aviatesk requested a review from vtjnash April 19, 2022 03:04
Copy link
Member

@vtjnash vtjnash left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These worlds are not sortable (they form a complex DAG, not a chain) so this won't be correct. We used to have a world_lowerbound lookup function in this file that we used to validate the lookup. In this case, I think finding a way to tag these as invoke calls would be better.

@KristofferC KristofferC mentioned this pull request Apr 19, 2022
67 tasks
@joa-quim
Copy link

Ping this.

@KristofferC KristofferC mentioned this pull request May 28, 2022
36 tasks
@timholy
Copy link
Member Author

timholy commented Jun 22, 2022

OK, I'm back to thinking about this (though it will be part-time at first). I don't know if I'll make it by julia-1.8.0 but perhaps we can backport it to a later 1.8.x release.

@vtjnash, thanks for correcting my mis-impressions. Here's my plan (copied from a slack discussion 😄 with @SimonDanisch), if you disagree feel free to chime in:

The solution will be to add another field to a MethodInstance, maybe invokedTypes (kind of like the specTypes field); if non-null that means it was called via an explicit invoke statement and that field stores the signature supplied to invoke. Then we check for invalidation against invokedTypes rather than against specTypes.

Quite simple in principle, but it may have extensive interacttions with the rest of the compiler. We'll have to store multiple copies of some MethodInstances, one for those called by normal dispatch and then another for each separate invoked signature. This means that we'll have to generalize our code-caching and other things, and there will probably be lots of bugs where old code that accesses specTypes really should first check invokedTypes but we have to find all those places...that's why it's going to take a while.

@vtjnash
Copy link
Member

vtjnash commented Jun 24, 2022

Ah, that is a good observation. So it sounds like this is an existing correctness bug, since the stored edge is too narrow. We need to have 2 edges stored and checked here: one from the dispatch signature (which could be represented as a MethodInstance for the invoke signature) and one from the compiled method (which ignores dispatch to it, but tracks changes to all of the code inside). For a regular (non-invoke) call, those 2 edges happen to land on the same method instance signature, so we only needed to record one of them.

@KristofferC KristofferC mentioned this pull request Jul 6, 2022
15 tasks
@vtjnash
Copy link
Member

vtjnash commented Jul 6, 2022

It also might fix #45837, if I understand this correctly

@timholy
Copy link
Member Author

timholy commented Jul 11, 2022

Interesting. xref #45051

@timholy
Copy link
Member Author

timholy commented Jul 21, 2022

@vtjnash, the approach you recommended has been implemented in #46010. Would love a review if you have time.

@timholy timholy removed the backport 1.8 Change should be backported to release-1.8 label Jul 21, 2022
@DilumAluthge DilumAluthge deleted the teh/invoke_invalidations_precompile branch September 3, 2022 18:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler:latency Compiler latency
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants