Remove fast paths when scaling arrays

[andreasnoack]

As pointed out in #29691 (comment). If the scaled array contains Inf or NaN then the fast path wasn't valid. This bug was also in BLAS so two array scaling methods no longer call BLAS for large arrays. I couldn't measure any slowdown. It's a very simple function so this isn't a surprise.

This reverts commit 0ba31aa but still fixes #29681.

[KristofferC]

-1 for removing explicit returns.

[iblislin]

I'm ok for both style. But I want the consistency of the same module.

[andreasnoack]

I added the return in the commit I'm reverting and I'm no longer touching that function. I also prefer explicit return statements.

[KristofferC]

Oops, sorry.

[KristofferC]

julia> A=randn(10^8);

julia> @time rmul!(A, 0.5);
  0.087498 seconds (4 allocations: 160 bytes)

julia> @time LinearAlgebra.generic_rmul!(A, 0.5);
  0.119145 seconds (4 allocations: 160 bytes)

suggests there is a performance difference?

[andreasnoack]

That is a long vector. I only tested up to 10^6.

julia> @btime LinearAlgebra.generic_rmul!(y, 0.4) setup=(y=randn(10^6));
  305.301 μs (0 allocations: 0 bytes)

julia> @btime BLAS.scal!(length(y), 0.4, y, 1) setup=(y=randn(10^6));
  380.453 μs (0 allocations: 0 bytes)

Maybe multithreading has a little effect for very long vectors.

[KristofferC]

I get

julia> @btime LinearAlgebra.generic_rmul!(y, 0.4) setup=(y=randn(10^6));
  439.046 μs (0 allocations: 0 bytes)

julia> @btime BLAS.scal!(length(y), 0.4, y, 1) setup=(y=randn(10^6));
  394.152 μs (0 allocations: 0 bytes)

on a 2015 MBP so I guess it depends on the cpu.

[andreasnoack]

I'll merge. There might be some architectures where BLAS is a bit faster that us but it looks like the opposite is also the case and for smaller arrays, I think we are generally as faster or faster. This is not an operation where we should expect BLAS to do better than LLVM.

[KristofferC]

Too big of a behavior change to backport.

[StefanKarpinski]

What is breaking about this change? The description and discussion make it seem like a removal of a unhelpful fast path but the "minor change" label and the fact that this seems to have broken BandedMatrices and BlockArrays indicates that it's breaking.

[fredrikekre]

Julia 1.0.2

julia> rmul!(fill(NaN, 2, 2), 0.0)
2×2 Array{Float64,2}:
 0.0  0.0
 0.0  0.0

while on master (due to this PR)

julia> rmul!(fill(NaN, 2, 2), 0.0)
2×2 Array{Float64,2}:
 NaN  NaN
 NaN  NaN

(which is the reason both of those packages fail).

[StefanKarpinski]

The new behavior seems like a bug fix...

[andreasnoack]

It is a bug fix. I'll take a look at the packages that break because of this to figure out what is going on.

[StefanKarpinski]

Even though it's a bug fix I think we should lean conservative with point releases and not backport.

[KristofferC]

This was removed from the backport list for 1.0.2 because it broke packages. (#29726 (reference))

[andreasnoack]

This broke a single test in each of the two packages we know are affected by this and the maintainer is aware of this change. The two tests specifically tested the buggy behavior, i.e. the failures weren't indirect from other functions that relied on this behavior. We are not aware of any other cases where this change has had an effect.

[KristofferC]

So what you are saying is that you want this in 1.0.3?

[andreasnoack]

Never mind. I read this as a discussion of exclusion from 1.1 and missed that Stefan said point release.