Remove permute!! and invpermute!!

[LilithHafner]

I don't think that these algorithms provide an unambiguous performance improvement that justifies their inclusion in base.

Perhaps a more complex dispatch system that looks at length and eltype might be warranted, but I'm in favor of excision and putting fancy and high-efficiency permutations somewhere like Permutations.jl. Here are some benchmarks that shed light on the consequences of this PR, green is an improvement and red is a regression:

Roguhly, I'm benchmarking [inv]permute!(Vector{NTuple{width, Int}}(undef, length), shuffle!(collect(1:length)))

Benchmark code

using Random, Plots
_permute!(a, p::AbstractVector) = copyto!(a, a[p])
_invpermute!(a, p::AbstractVector) = copyto!(a, a[invperm(p)])

function t(::Type{T}, n, fs...) where T
    m = max(1, 10^6 ÷ max(10, n * sizeof(T)))
    A = [Vector{T}(undef, n) for _ in 1:m]
    P = [shuffle!(collect(1:n)) for _ in 1:m]
    [@elapsed(for (a,p) in zip(A,P) 
        f(a,p)
    end) for f in fs]
    [@elapsed(for (a,p) in zip(A,P) 
        f(a,p)
    end) for f in fs]
end

function compute()
    global fs = permute!,_permute!,invpermute!,_invpermute!
    global data = [t(NTuple{x,Int},2^y,fs...) 
        for x in 1:15, y in 1:20]
end
function plot()
    for (i,f) in collect(enumerate(fs))[[1,3]]
        display(heatmap(
            log10.(getindex.(data, i+1) ./ getindex.(data, i)),
            c=cgrad([:black, :green, :white, :red, :black]),
            xlabel="log2(length)", ylabel="width",
            title="log10(_$f / $f)",
            clims=(-2,2)))
    end
end

[StefanKarpinski]

Ah, I had marked this as a 2.0 change because I thought these were exported but they aren't so we could remove them in a minor release.

[stevengj]

Note that in practice, there are several packages using Base.permute!!: DataArrays, StructArrays, DataFrames, IndexedTables, PooledArrays, … so this change will be breaking.

[LilithHafner]

Good catch, @stevengj.

Going through all the packages that use Base.permute!! or Base.ipermute!!

DataArrays has been deprecated since julia 1.0
DataFrames no longer uses Base.permute!! in its master branch, it now uses a much faster approach.
PooledArrays only uses Base.permute!! to define Base.permute!! on the PooledArray type.

Some packages would need to switch from Base.permute!! to permute!. All of these packages use Base.permute!! instead of permute! only for performance reasons but may experience a speedup switching to permute! after this PR compared to the current Base.permute!!.

Clustering would almost certainly see a speedup as the eltype it's permuting is Int.
IndexdTables could be much (perhaps 2x) faster if it used a similar approach as DataFrames instead of Base.permute!!, whether it sees a speedup depends on user eltype size and length.
Qaintessent may see a minor regression.
StructArrays uses Base.permute!! on an eltype of references, so I suspect it would also be faster after this PR to simply call permute!.

If folks want to go through with this PR, I can make the necessary PRs to those packages.

[oscardssmith]

If these PRs are expected to be performance wins, it sounds like they should be made whether we want this PR or not.

[LilithHafner]

The performance wins are of the form "permute! after PR is better than Base.permute!! which is better than permute! before PR", so without this PR they would have to switch to copyto!(v, v[p]) rather than permute!(v, p), that aside, yes.

[oscardssmith]

In that case, I think the order should be

1. fix performance of permute! / invpermute!.
2. make PRs to packages.
3. decide whether we want to deprecate permute!!

[LilithHafner]

Step 1 is ready for review.

[LilithHafner]

Waiting for the release of 1.9 to begin step 2.

[alyst]

[inv]permute!!(v, p) doesn't allocate the new array, so while the algorithm itself might be slower than copyto!(v, v[p]), in certain contexts the GC overhead of permute!() may result in lower overall performance.

[LilithHafner]

The benchmarks in the OP do not exclude GC overhead.

[alyst]

The benchmarks in the OP do not exclude GC overhead.

They do not, but IIUC they don't test for cases where v and/or p are reused 1000x times.

[LilithHafner]

I'm not going to reproduce the above figure with 1000x iterations because that would take to long. Here's a single point from the figure in the OP, benchmarked while reusing both parameters 1000x times. This result is consistent with the figures in the OP.

julia> x2 = rand(1000); perm = Vector{Int}(undef, 1000); perm2 = Vector{Int}(undef, 1000); @benchmark Base.permute!!($x2, copyto!($perm2, perm)) setup=(rand!($x2); randperm!($perm)) gcsample=false gctrial=false evals=1000 samples=100
BenchmarkTools.Trial: 100 samples with 1000 evaluations.
 Range (min … max):  2.932 μs …   4.214 μs  ┊ GC (min … max): 0.00% … 0.00%
 Time  (median):     3.284 μs               ┊ GC (median):    0.00%
 Time  (mean ± σ):   3.293 μs ± 215.969 ns  ┊ GC (mean ± σ):  0.00% ± 0.00%

       ▃▃ ▃  ▃▁ ▁ ▁▁▁  ▆█▆▆ ▃▆▃▆                               
  ▄▄▄▁▄██▄█▁▄██▁█▁███▇▁████▇████▇▇▁▄▄▄▇▄▁▁▄▄▁▁▁▁▁▁▄▁▄▄▁▁▁▁▇▁▇ ▄
  2.93 μs         Histogram: frequency by time        3.83 μs <

 Memory estimate: 0 bytes, allocs estimate: 0.

julia> x2 = rand(1000); perm = Vector{Int}(undef, 1000); perm2 = Vector{Int}(undef, 1000); @benchmark Base.permute!($x2, copyto!($perm2, perm)) setup=(rand!($x2); randperm!($perm)) gcsample=false gctrial=false evals=1000 samples=100
BenchmarkTools.Trial: 100 samples with 1000 evaluations.
 Range (min … max):  1.018 μs … 8.594 μs  ┊ GC (min … max):  0.00% … 79.15%
 Time  (median):     1.323 μs             ┊ GC (median):     0.00%
 Time  (mean ± σ):   1.730 μs ± 1.108 μs  ┊ GC (mean ± σ):  22.74% ± 23.38%

    █                                                        
  ▄▄██▄▃▂▂▃▃▁▁▁▁▁▃▁▄▃▂▁▂▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▂▁▁▁▁▁▁▁▁▂ ▂
  1.02 μs        Histogram: frequency by time       6.83 μs <

 Memory estimate: 7.94 KiB, allocs estimate: 1.

[stevengj]

Note that these follow-the-cycles algorithms become more of a win when the elements size is large, as in Base.permutecols!! where we are permuting the columns of a matrix (used for sorting eigenvectors).

[PallHaraldsson]

If you want to go ahead with this, then I agree it should be dropped ASAP in 1.11/master, early in 1.11 development, in case we need to revert it later.

We shouldn't backport this to 1.10

If you drop this in 1.11 I would argue doing it in 1.10 too before its release, in case it will be LTS. It would be easy to add it back in 1.10.1, but if it stays in then it will be a (practically) breaking change to drop in in a 1.10.x, seemingly bad for an LTS.

[LilithHafner]

888 packages failed tests only on the current version.

😢

[LilithHafner]

After Rerunning after JuliaRegistries/General#91067
@nanosoldier runtests()

[nanosoldier]

The package evaluation job you requested has completed - possible new issues were detected.
The full report is available.

[LilithHafner]

@nanosoldier runtests(["PooledArrays"], configuration=(registry="LilithHafner/General#permute-testing", ))

[nanosoldier]

The package evaluation job you requested has completed - possible new issues were detected.
The full report is available.

[LilithHafner]

@nanosoldier runtests(["PooledArrays"], configuration=(registry="LilithHafner/General#permute-testing", ), vs = ":master")

[LilithHafner]

Trying again after fixing a bug in my custom registry setup:
@nanosoldier @nanosoldier runtests(["PooledArrays"], configuration=(registry="LilithHafner/General#permute-testing", ), vs = ":master")

[nanosoldier]

The package evaluation job you requested has completed - possible new issues were detected.
The full report is available.

[LilithHafner]

tehe, lots of noise and trial and error, but not too much load on the nanosoldier machines as I experiment with this. I think my most recent invocation was ignored because I miscopied it.

@nanosoldier runtests(["PooledArrays"], configuration=(registry="LilithHafner/General#permute-testing", ), vs = ":master")

[nanosoldier]

The package evaluation job you requested has completed - possible new issues were detected.
The full report is available.

[LilithHafner]

@nanosoldier runtests(["PooledArrays"])

[nanosoldier]

The package evaluation job you requested has completed - no new issues were detected.
The full report is available.

[LilithHafner]

@nanosoldier runtests()

[nanosoldier]

The package evaluation job you requested has completed - possible new issues were detected.
The full report is available.

[LilithHafner]

Failures due to folks using old versions of PooledArrays and StructArrays. I'll try again in a while.

[LilithHafner]

Eh, whatever. This is good enough. The implementations are gone and the methods are deprecated.