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

Performance/type inference regression from #12327 #12476

Closed
simonster opened this issue Aug 5, 2015 · 3 comments
Closed

Performance/type inference regression from #12327 #12476

simonster opened this issue Aug 5, 2015 · 3 comments
Labels
performance Must go faster regression Regression in behavior compared to a previous version

Comments

@simonster
Copy link
Member

It occurs to me that, in the heterogeneous key/value type case, #12327/#12261 causes us to lose type information. Consider basically any code that uses the for (k, v) in dict idiom, e.g.:

function f(a)
    x = 0
    for (k, v) in a
        x += v
    end
    x
end

Now consider what happens if the key and value types aren't the same. Previously, because we do fancy things for tuple destructuring, we could infer that k is of the key type K and v is of the value type V. But we don't do these fancy things for Pair destructuring. code_warntype(f, (Dict{Float64,Int},)) shows that now we can only determine that k and v are both of type Union{K,V}. For the benchmark:

d = Dict([rand() => rand(Int) for i = 1:10000000]);
@time f(d)

On 6c34bc7 I get:

  0.476170 seconds (30.00 M allocations: 457.764 MB, 6.70% gc time)

On the 0.3 branch I get:

elapsed time: 0.127657036 seconds (96 bytes allocated)
@simonster simonster added performance Must go faster regression Regression in behavior compared to a previous version labels Aug 5, 2015
@carnaval
Copy link
Contributor

carnaval commented Aug 5, 2015

As a short term solution you could add an indexed_next thing for pairs, as well as the corresponding hack in inference.jl.

I have some ideas on how to solve this more generally (this is the same problem as the getindex(::Fact,::Symbol) non leaf return types for matrix factorizations) but it's not gonna happen soon :-)

@StefanKarpinski
Copy link
Member

Would it help of Pair were defined this way instead:

immutable Pair{A,B}
    pair::Tuple{A,B}
end

That way the Pair type might inherit some of the fancy destructuring for tuples.

@simonster
Copy link
Member Author

That might work, but it might not be great for non-isbits since it's an extra object and pointer indirection.

simonster added a commit that referenced this issue Aug 6, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance Must go faster regression Regression in behavior compared to a previous version
Projects
None yet
Development

No branches or pull requests

3 participants