Fix map for Julia v0.7

[andyferris]

This works around a redefinition of map for all AbstractArray that occurs on v0.7 but not v0.6.

The newly-missed optimization opportunities are probably pretty rare in practice.

[andyferris]

Fixes #282. Hopefully fixes #291.

[coveralls]

Coverage decreased (-0.2%) to 92.966% when pulling 603ee5e on andyferris-fix-map into c7d2b2d on master.

[c42f]

Aww, this was such a nice hack.

But yes, we need to fix this, and I think a "proper" fix to get the 0.6 behaviour will need to involve Base...

[codecov-io]

Codecov Report

Merging #296 into master will decrease coverage by 0.03%.
The diff coverage is 50%.

@@            Coverage Diff             @@
##           master     #296      +/-   ##
==========================================
- Coverage   93.15%   93.11%   -0.04%     
==========================================
  Files          36       36              
  Lines        2629     2630       +1     
==========================================
  Hits         2449     2449              
- Misses        180      181       +1

Impacted Files	Coverage Δ
src/mapreduce.jl	99.18% <50%> (-0.82%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c7d2b2d...70ed38f. Read the comment docs.

[timholy]

How about

map(f, a1::StaticArray, as::AbstractArray...)
    _map(f, same_size(a1, as...), a1, as...)
end

And, do you really need @inline on 0.7? One can make the argument that now that we have an inliner that's supposed to be smart enough to figure this stuff out on its own (JuliaLang/julia#22210, JuliaLang/julia#22775), if you need @inline, then it's a bug. I still find @noinline sometimes handy (you can pick and choose where you want GC frames to be allocated), but I'm not convinced I've ever found a case where @inline helpful, and sometimes it's counterproductive (avoiding it can save you some insane compilation times, e.g., ntuple(identity, Val(1000)).

[c42f]

Yes, this would be less surprising than completely ditching the mixed Static vs Abstract array cases, I suppose.

Regarding how this interacts with Base, I was wondering whether we should have some machinery like that used in Broadcast for determining the output container type, which could solve this problem in general. Any thoughts on feasibility?

[andyferris]

@timholy I have tried to ask before whether the new inliner takes into account the fact that the overhead of the call is proportional to the length of the arguments. My understanding is that in StaticArrays we get improvements for inlining operations on large static arrays because of this fact.

Case in point:

julia> @generated function _map(f, t::Tuple)
           exprs = [:(f(t[$i])) for i = 1:length(t.parameters)]
           return quote tuple($(exprs...)) end
       end
_map (generic function with 1 method)

julia> f(x) = _map(x->2*x, x)
f (generic function with 1 method)

julia> @code_warntype f(ntuple(identity,3))
Variables:
  #self# <optimized out>
  x::Tuple{Int64,Int64,Int64}
  #33 <optimized out>
  #temp#::Tuple{Int64,Int64,Int64}

Body:
  begin
      # meta: location REPL[17] _map 2
      # meta: location REPL[17] @generated body
      #= line 3 =#
      #temp#::Tuple{Int64,Int64,Int64} = (Main.tuple)((Base.mul_int)(2, (Base.getfield)(x::Tuple{Int64,Int64,Int64}, 1, true)::Int64)::Int64, (Base.mul_int)(2, (Base.getfield)(x::Tuple{Int64,Int64,Int64}, 2, true)::Int64)::Int64, (Base.mul_int)(2, (Base.getfield)(x::Tuple{Int64,Int64,Int64}, 3, true)::Int64)::Int64)::Tuple{Int64,Int64,Int64}
      goto 7
      # meta: pop location
      7: 
      # meta: pop location
      return #temp#::Tuple{Int64,Int64,Int64}
  end::Tuple{Int64,Int64,Int64}

julia> @code_warntype f(ntuple(identity,30))
Variables:
  #self# <optimized out>
  x::NTuple{30,Int64}
  #33::getfield(Main, Symbol("##33#34"))

Body:
  begin
      #33::getfield(Main, Symbol("##33#34")) = $(Expr(:new, :(Main.##33#34)))
      return $(Expr(:invoke, MethodInstance for _map(::getfield(Main, Symbol("##33#34")), ::NTuple{30,Int64}), :(Main._map), :(#33), :(x)))::NTuple{30,Int64}
  end::NTuple{30,Int64}

Generally, we've been supporting static arrays up to 16x16 or so (tuples of length 256) and I've tried to avoid invoke in the cases where the operation is O(N). I guess I should do more benchmarking to support my case here since perhaps I'm worrying about the wrong thing... but I hope you can see what I am worried about.

[timholy]

Model is currently fixed cost per :invoke, but that would not be hard to change. Would need some benchmarks though.

[andyferris]

Cool, yes I'm happy to work together on this stuff and will make some benchmarks (when I have some time!!). Did you have any test cases set up from your work on this to-date?

FYI - I do expect that we will majorly update StaticArrays for v1.0 so hopefully we can get rid of all those pesky @inlines by then :)

[timholy]

Did you have any test cases set up from your work on this to-date?

I just used BaseBenchmarks, nothing specific.

[coveralls]

Coverage decreased (-0.04%) to 93.118% when pulling 70ed38f on andyferris-fix-map into c7d2b2d on master.

[ararslan]

Any chance you could tag a new release now that this is merged?

[c42f]

@ararslan does this fix the massive breakage you were observing on 0.7?

[ararslan]

Actually I'm not sure yet. The breakage was with Distributions, but there seems now to be issues with other packages that are keeping Distributions from loading.

[timholy]

In conjunction with some other local changes, this is enough to let me build ImageFiltering.jl now (for the first time in ages). 👍