-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Reduce allocations in broadcast #19639
Conversation
e55ed1c
to
4134488
Compare
Are the changes to the sparse matrix code related to the addressed problem? |
@nanosoldier |
Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @jrevels |
@@ -1403,13 +1403,19 @@ sparse(S::UniformScaling, m::Integer, n::Integer=m) = speye_scaled(S.λ, m, n) | |||
# map/map! entry points | |||
function map!{Tf,N}(f::Tf, C::SparseMatrixCSC, A::SparseMatrixCSC, Bs::Vararg{SparseMatrixCSC,N}) | |||
_checksameshape(C, A, Bs...) | |||
return map_nocheck!(f, C, A, Bs...) |
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.
Maybe this could be tied to the bounds checking mechanism? Or would it be an abuse?
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.
This looks great!
Perhaps having @timholy
sign off on the inlining changes would be prudent?
The broadcast-fusion and linalg-arithmetic benchmark improvements are lovely. The scalar-floatexp-ldexp, sparse-arithmetic-unary minus, and string-join regressions should be noise. Might the linalg-factorization and array regressions be real?
I agree with @martinholters
, the sparse matrix changes are orthogonal to the other changes in this pull request. I would prefer those changes appear in a separate pull request. (I might advocate holding off with that pull request for now, having left that TODO outstanding for two reasons: I wasn't certain whether avoiding the redundant shape check is worth the extra code complexity, and I plan to restructure that code somewhat in the near future in any case.)
Thanks again @pabloferz!
4134488
to
361db25
Compare
I removed the sparse related changes. The initial changes seemed to affect somehow some the The reason for which there was a |
function broadcast_t(f, ::Type{Any}, T::Type, shape, iter, As...) | ||
if isempty(iter) | ||
return similar(Array{T}, shape) | ||
end |
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.
Why move the code handling the empty case inside this method and add a second type argument?
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.
Ups. I was playing around reorganizing the code and left this, but shouldn't be necessary. I'll put it back as it was.
361db25
to
4c964d3
Compare
@nanosoldier |
Your benchmark job has completed - no performance regressions were detected. A full report can be found here. cc @jrevels |
Combined with dot ops, we now have: julia> function bar(x, n)
for i = 1:n
x .= 2 .* x .+ 1
end
return x
end
bar (generic function with 1 method)
julia> @time bar([0,0,0], 10^4); # warmup
0.020100 seconds (17.47 k allocations: 713.317 KB)
julia> @time bar([0,0,0], 10^4);
0.000226 seconds (6 allocations: 288 bytes) |
With this PR
Compare this with #19608 (comment) and #16285 (comment)