slow unidimensional circshift on dense (small) vectors

[abraunst]

on master:

julia> x=rand(10);y=similar(x);

julia> @benchmark circshift!($y,$x,1)

BenchmarkTools.Trial: 
  memory estimate:  144 bytes
  allocs estimate:  3
  --------------
  minimum time:     785.802 ns (0.00% GC)
  median time:      795.614 ns (0.00% GC)
  mean time:        927.788 ns (8.73% GC)
  maximum time:     609.764 μs (99.84% GC)
  --------------
  samples:          10000
  evals/sample:     101

julia> @benchmark circshift!($y,$x,(1,))

BenchmarkTools.Trial: 
  memory estimate:  0 bytes
  allocs estimate:  0
  --------------
  minimum time:     60.283 ns (0.00% GC)
  median time:      61.008 ns (0.00% GC)
  mean time:        63.170 ns (0.00% GC)
  maximum time:     516.131 ns (0.00% GC)
  --------------
  samples:          10000
  evals/sample:     982

(note the allocations in the first case). With this minimal PR:

1.1.0-DEV.841> @benchmark circshift!($y,$x,1)
BenchmarkTools.Trial: 
  memory estimate:  0 bytes
  allocs estimate:  0
  --------------
  minimum time:     65.270 ns (0.00% GC)
  median time:      67.583 ns (0.00% GC)
  mean time:        68.780 ns (0.00% GC)
  maximum time:     240.412 ns (0.00% GC)
  --------------
  samples:          10000
  evals/sample:     981

Are there ill effects of the patch? As I see it, enlargement of tuples is already treated separately, so the only remaining case is single integer to tuple, so it seems that there is no need for the splat.

This was hurting #30317, by the way. (Now it's better than in master even for dense matrices in sparse representation)

[mbauman]

See #29114 for the root issue here. It's worth plugging this hole manually, but we'll want to be careful that we don't remove functionality (like the ability to pass other iterable collections) or introduce dispatch ambiguities. It's a bit of a hodgepodge of an "interface" (if you want to call it that), but if someone had implemented circshift!(dest::CustomArray, src, n), then they'll be prone to an ambiguity if we add this definition. I think an internal helper function would avoid both problems here.

[abraunst]

See #29114 for the root issue here. It's worth plugging this hole manually, but we'll want to be careful that we don't remove functionality (like the ability to pass other iterable collections) or introduce dispatch ambiguities. It's a bit of a hodgepodge of an "interface" (if you want to call it that), but if someone had implemented circshift!(dest::CustomArray, src, n), then they'll be prone to an ambiguity if we add this definition. I think an internal helper function would avoid both problems here.

Ah I see... Would something like the updated PR be ok?

EDIT: there's also some asymmetry between circshift and circshift! and it may seem that circshift suffers from the problem you mentioned (or am I missinterpreting)?

1.1.0-DEV.841> methods(circshift)
# 3 methods for generic function "circshift":
[1] circshift(a::AbstractArray, shiftamt::Real) in Base at abstractarraymath.jl:182
[2] circshift(a::AbstractArray, shiftamt::Tuple{Vararg{Integer,N}} where N) in Base at abstractarraymath.jl:184
[3] circshift(a::AbstractArray, shiftamt) in Base at abstractarraymath.jl:243

1.1.0-DEV.841> methods(circshift!)
# 5 methods for generic function "circshift!":
[1] circshift!(dest::BitArray{1}, src::BitArray{1}, i::Integer) in Base at bitarray.jl:1328
[2] circshift!(B::BitArray{1}, i::Integer) in Base at bitarray.jl:1344
[3] circshift!(dest::AbstractArray, src, ::Tuple{}) in Base at multidimensional.jl:917
[4] circshift!(dest::AbstractArray{T,N}, src, shiftamt::Tuple{Vararg{Integer,N}} where N) where {T, N} in Base at multidimensional.jl:930
[5] circshift!(dest::AbstractArray, src, shiftamt) in Base at multidimensional.jl:935