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

add some type info to Base to avoid excess recursion in inference #33476

Merged
merged 1 commit into from
Oct 7, 2019

Conversation

JeffBezanson
Copy link
Member

Fixes #33336.

This addresses some commonly-occurring cases where having too little type info makes inference see a lot of recursion in Base that is not actually possible.

Of course, this is not satisfying, since these are empirically determined (*). @vtjnash & I would also like to make inference's cycle handling smarter, but that will be a longer project.

Needs review from @mbauman : Are the dataid arguments to _isdisjoint indeed always UInt? And is it safe to assume that the length of an index in broadcast is an Integer?

(*) Methodology is as follows:
Apply this patch:

--- a/base/compiler/inferencestate.jl
+++ b/base/compiler/inferencestate.jl
@@ -245,6 +245,10 @@ function add_mt_backedge!(mt::Core.MethodTable, @nospecialize(typ), caller::Infe
 end
 
 function poison_callstack(infstate::InferenceState, topmost::InferenceState, poison_topmost::Bool)
+    if trace_inf[1]
+        print_callstack(infstate)
+        println()
+    end

(you will also need to add const trace_inf = [false] somewhere. Set Core.Compiler.trace_inf[]=true just before running the problematic call. In the output, look for calls from simple "innocent" Base functions that lead into package code or big nests of code like broadcast, that they should not be able to touch. For example:

#broadcastMD#59(Base.Iterators.Pairs{Union{}, Union{}, Tuple{}, NamedTuple{(), Tuple{}}}, typeof(MDDatasets.broadcastMD), MDDatasets.CastType2{Number, 1, Number, 2}, typeof(Base.:(>)), Int64, MDDatasets.DataHR{T2} where T2)  [limited]
broadcastMD(MDDatasets.CastType2{Number, 1, Number, 2}, typeof(Base.:(>)), Int64, MDDatasets.DataHR{T2}) where {T2}  [limited]
>(Int64, MDDatasets.DataMD)  [limited]
fill_to_length(Tuple{Integer, Vararg{Any, N} where N}, Int64, Base.Val{_A} where _A)  [limited]
(::Type{Base.IteratorsMD.CartesianIndex{_A}})(Tuple{Integer, Vararg{Any, N} where N})  [limited]

There, fill_to_length, a low-level tuple utility, led to inference of >(Int64, MDDatasets.DataMD) which then calls broadcast, etc. That can be patched up by declaring the second argument to > in fill_to_length as Int (which it will always be, but inference can't tell).

base/namedtuple.jl Outdated Show resolved Hide resolved
Fixes #33336.
This addresses some commonly-occurring cases where having too little
type info makes inference see a lot of recursion in Base that is
not actually possible.
Copy link
Member

@mbauman mbauman left a comment

Choose a reason for hiding this comment

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

Approve on both counts; data_ids only ever returns UInt (and is documented to do such), and I would be very surprised if broadcast could work with non-Integer lengths (and length is documented to only return Integers).

@JeffBezanson JeffBezanson merged commit d5d5718 into master Oct 7, 2019
@JeffBezanson JeffBezanson deleted the jb/cutrecursionedges branch October 7, 2019 19:33
KristofferC pushed a commit that referenced this pull request Oct 8, 2019
…3476)

Fixes #33336.
This addresses some commonly-occurring cases where having too little
type info makes inference see a lot of recursion in Base that is
not actually possible.

(cherry picked from commit d5d5718)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler:inference Type inference compiler:latency Compiler latency
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Inference performance regression vs 1.1 (MDDatasets.jl)
4 participants