-
Notifications
You must be signed in to change notification settings - Fork 149
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 map
for Julia v0.7
#296
Conversation
This works around a redefinition of `map` for all `AbstractArray` that occurs on v0.7 but not v0.6. This newly-missed optimization opportunities are probably pretty rare in practice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 Report
@@ 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
Continue to review full report at Codecov.
|
src/mapreduce.jl
Outdated
_map(f, same_size(as...), as...) | ||
end | ||
else | ||
@inline function map(f, as::StaticArray) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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))
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Model is currently fixed cost per :invoke
, but that would not be hard to change. Would need some benchmarks though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 @inline
s by then :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you have any test cases set up from your work on this to-date?
I just used BaseBenchmarks, nothing specific.
Any chance you could tag a new release now that this is merged? |
@ararslan does this fix the massive breakage you were observing on 0.7? |
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. |
In conjunction with some other local changes, this is enough to let me build ImageFiltering.jl now (for the first time in ages). 👍 |
This works around a redefinition of
map
for allAbstractArray
that occurs on v0.7 but not v0.6.The newly-missed optimization opportunities are probably pretty rare in practice.