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

Allow limit_type_depth to introduce more than one new TypeVar #20626

Merged
merged 4 commits into from
Feb 20, 2017

Conversation

martinholters
Copy link
Member

@martinholters martinholters commented Feb 16, 2017

Fixes #20615.

This is WIP because it increases the time to run the fft test from 20s to 630s on my system. (All other tests look unsuspicious at first glance.)

@ararslan ararslan added the types and dispatch Types, subtyping and method dispatch label Feb 16, 2017
@martinholters
Copy link
Member Author

Looks like the foldr was the culprit. Spelling the loop out is vastly faster (at least for the fft test). No idea what's special about that test and why foldr is so slow there.

@martinholters martinholters changed the title WIP: Allow limit_type_depth to introduce more than one new TypeVar Allow limit_type_depth to introduce more than one new TypeVar Feb 17, 2017
@martinholters
Copy link
Member Author

I wonder whether types deep enough for this PR to make a difference appear in the benchmarks, but better be safe than sorry, so @nanosoldier runbenchmarks(ALL, vs = ":master")

@martinholters
Copy link
Member Author

Should we do something like

--- a/base/inference.jl
+++ b/base/inference.jl
@@ -660,7 +660,13 @@ function type_depth(t::ANY)
     return 0
 end
 
-function limit_type_depth(t::ANY, d::Int, cov::Bool=true, vars::Vector{TypeVar}=TypeVar[])
+function limit_type_depth(t::ANY, d::Int)
+    r = limit_type_depth(t, d, true, TypeVar[])
+    @assert t <: r
+    return r
+end
+
+function limit_type_depth(t::ANY, d::Int, cov::Bool, vars::Vector{TypeVar}=TypeVar[])
     if isa(t,Union)
         if d > MAX_TYPE_DEPTH
             return Any

to turn potentially very subtle bugs into more obvious ones? (Of course, limit_type_depth will be absolutely free of any bugs with this PR, but just to be sure... 😄)

@nanosoldier
Copy link
Collaborator

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @jrevels

@JeffBezanson
Copy link
Sponsor Member

Nice, 👍.

Only one small thing; I would not use gensym here since it might generate a large number of symbols that aren't strictly necessary. Yes, using separate names makes the types easier to read, but we can use a symbol pool instead.

@martinholters
Copy link
Member Author

I'd like to do something like

var = TypeVar(Symbol("_", length(vars) + 1), t.name.wrapper)

but string concatenation is not available during inference. Is there any easy way to accomplish this? With a "symbol pool", you probably meant having something like [:_1, :_2, ..., :_9] around and then cycling through it to achieve basically the same?

My current thinking is: Either go all the way and ensure that the TypeVars have different Symbols so that they parse as they print - but without gensym() that might be more complicated than is justified, or just use :_ throughout, which I've done now.

@JeffBezanson JeffBezanson merged commit eb504bb into master Feb 20, 2017
@StefanKarpinski StefanKarpinski deleted the mh/fix_ltd branch February 20, 2017 21:22
@TotalVerb
Copy link
Contributor

Scott reported a problem, possibly related to this (as it seems to trigger the assertion here), on Discourse: https://discourse.julialang.org/t/strange-failure-in-v0-6/2911/8

return r
end

function limit_type_depth(t::ANY, d::Int, cov::Bool, vars::Vector{TypeVar}=TypeVar[])
if isa(t,Union)
if d > MAX_TYPE_DEPTH
return Any
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this have been cov ? Any : (v = TypeVar(:_); push!(vars, v); v)? cc @martinholters

Copy link
Member Author

Choose a reason for hiding this comment

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

Sounds reasonable. I'll look into this...

Copy link
Member Author

Choose a reason for hiding this comment

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

PR at #21192, but also needs #21191 fixed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
types and dispatch Types, subtyping and method dispatch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants