Skip to content
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

View allocation elimination stumped by conditional? #28226

Closed
KristofferC opened this issue Jul 22, 2018 · 6 comments
Closed

View allocation elimination stumped by conditional? #28226

KristofferC opened this issue Jul 22, 2018 · 6 comments
Labels
performance Must go faster regression Regression in behavior compared to a previous version

Comments

@KristofferC
Copy link
Member

KristofferC commented Jul 22, 2018

Tracked down after performance report on Distances.jl (JuliaStats/Distances.jl#100)

Code to reproduce:

module Distances2

function colwise!(r, a, b)
    @inbounds for j = 1:size(a,2)
        r[j] = evaluate(view(a, :, j), view(b, :, j))
    end
    r
end

@inline function evaluate(a, b)
    length(a) == 0 && return 0.0 # comment out and 0.7 is super fast
    @inbounds begin
        s = 0.0
        @simd for I in eachindex(a, b)
            ai = a[I]
            bi = b[I]
            s += abs2(ai - bi)
        end
        return s
    end
end

end

using BenchmarkTools
z = zeros(41); A = rand(2, 41); B = rand(2, 41);
@btime Distances2.colwise!($z, $A, $B);

On 0.7:

 562.371 ns (82 allocations: 3.84 KiB)

On 0.6:

 255.466 ns (0 allocations: 0 bytes)

Now, if we comment away the length(a) == 0 && return 0.0 line:

0.7:

100.298 ns (0 allocations: 0 bytes)

0.6:

 206.193 ns (0 allocations: 0 bytes)
@KristofferC KristofferC added performance Must go faster regression Regression in behavior compared to a previous version labels Jul 22, 2018
@KristofferC
Copy link
Member Author

Actually, changing out eachindex(a, b) to 1:size(a, 2) also seems to help.

@KristofferC KristofferC added the potential benchmark Could make a good benchmark in BaseBenchmarks label Jul 22, 2018
@Keno
Copy link
Member

Keno commented Jul 22, 2018

Smells like the inlining heuristic change.

@KristofferC
Copy link
Member Author

I checked out the commit before #27857 and it didn't change anything.

@KristofferC
Copy link
Member Author

KristofferC commented Jul 23, 2018

In fact, manually inlining shows the same regression:

using BenchmarkTools

function colwise!(r, A, B)
    @inbounds for j = 1:size(A,2)
        a = view(A, :, j)
        b = view(B, :, j)
        r[j] = begin
            if #= false && =# length(a) == 0  # Remove comment and fast
                0.0
            else
                @inbounds begin
                    s = 0.0
                    for I in eachindex(a, b)
                        ai = a[I]
                        bi = b[I]
                        s += abs2(ai - bi)
                    end
                    s
                end
            end
        end
    end
    r
end


using BenchmarkTools
z = zeros(41); A = rand(2, 41); B = rand(2, 41);
@btime colwise!($z, $A, $B);

With and without false commented out respectively

  115.017 ns (0 allocations: 0 bytes)

  589.823 ns (82 allocations: 3.84 KiB)

So I think inlining can be ruled out.

@KristofferC
Copy link
Member Author

KristofferC commented Jul 23, 2018

Changing the if to a ifelse makes the allocation go away, I will use that to work around the original issue.

Edit: Not enough on the original issue though. Removing @simd make allocations go away (but then no simd).....

Edit2: The final fix was to change eachindex(a,b) to 1:length(a) which was safe in this specific case (https://github.com/JuliaStats/Distances.jl/pull/101/files#diff-08d86a8ef0a417f2c2269c73f1dbfe5b).

@KristofferC
Copy link
Member Author

Fixed, now zero allocations.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance Must go faster regression Regression in behavior compared to a previous version
Projects
None yet
Development

No branches or pull requests

2 participants