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

Fix more invalidations from overloading == #36282

Merged
merged 5 commits into from
Jun 16, 2020
Merged

Conversation

timholy
Copy link
Member

@timholy timholy commented Jun 14, 2020

Other than a PR to Pkg (which needs #36280), this is pretty much everything that I think can be done on #36005 without changes to the compiler itself. The combination of #36255, #36280, #36281, the upcoming Pkg PR, some PRs to JuliaInterpreter/LoweredCodeUtils/Revise, and this PR is to reduce the total count of (non-unique) invalidations from more than 3k to 529. Of those, 281 are from ==(::Symbol, ::Any), which I do not see a reasonable way to address without changes to the compiler itself (some details were mentioned in #36255).

@timholy timholy added the compiler:latency Compiler latency label Jun 14, 2020
@timholy
Copy link
Member Author

timholy commented Jun 15, 2020

BTW, we are making progress but there's still a lot of work ahead. Numbers in the left two columns are where we were in May (around the time of the 1.5 feature freeze) when I was actively working on the blog post, numbers in the right two columns are where we are now on this PR and all the ones referenced above:

Package more specific (May) ambiguous (May) more specific (now) ambiguous (now)
Example 0 0 0 0
Revise 7 0 0 0
FixedPointNumbers 170 387 34 470
SIMD 3903 187 5 874
StaticArrays 989 3133 593 2060
Optim 1643 2921 760 2103
Images 1749 3671 610 2515
Flux 1991 3460 not loadable (CUDAnative)
Plots 1542 4302 1268 3028
DataFrames 4919 783 3520 1833
JuMP 2145 4670 not loadable (MutableArithmetics)
Makie 6233 5526 710 4081
DifferentialEquations 5152 6218 3787 3432

Obviously, the single biggest advance will come when the ambiguous cases go away, but #36200 and all these PRs to improve inferrability are starting to have a pretty big cumulative effect. @SimonDanisch and @eschnett should be particularly pleased.

I'm not quite sure why the ambiguity numbers have changed so much, but either it's changes in the packages (I wasn't careful to insist on the same versions) or the changes in inference have changed the dependency trees in a way that affects the ambiguity cases, too.

Once we stop having ambiguous cases be invalidating, some of the MethodInstances in the "ambiguous" column will move to the "more specific" column. The analysis above counts each MethodInstance just once, but a given MethodInstance can be invalidated for more than one reason. If fc has a huge number of callers and itself calls both fa and fb, and if fa gets invalidated due to ambiguity before fb gets invalidated due to morespecific, all of fc's callers will be counted as having been invalidated due to an ambiguity. Once the ambiguities stop triggering invalidations, fc will still be invalidated due to fb and all those counts will shift from ambiguity to morespecific. Nevertheless, this is still progress, because it's one less reason to invalidate fc. Moreover, now there's extra motivation to figure out how to avoid invalidating fb, because if you do that suddenly you may no longer invalidate fc at all.

base/cartesian.jl Outdated Show resolved Hide resolved
@timholy
Copy link
Member Author

timholy commented Jun 15, 2020

Wow, that downloads test fails a lot...

@JeffBezanson JeffBezanson merged commit 2749582 into master Jun 16, 2020
@JeffBezanson JeffBezanson deleted the teh/more_inval branch June 16, 2020 17:33
simeonschaub pushed a commit to simeonschaub/julia that referenced this pull request Aug 11, 2020
* Improve typing of ProcessGroup.refs

* Add type annotation in stacktrace handling

* Eliminate boxing in REPL

Related to JuliaLang#15276
Keno pushed a commit that referenced this pull request Jun 5, 2024
* Improve typing of ProcessGroup.refs

* Add type annotation in stacktrace handling

* Eliminate boxing in REPL

Related to #15276
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.

2 participants