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

performance of captured variables in closures #15276

Open
timholy opened this Issue Feb 28, 2016 · 70 comments

Comments

@timholy
Copy link
Member

timholy commented Feb 28, 2016

using Images: realtype

function ifi{T<:Real,K,N}(img::AbstractArray{T,N}, kern::AbstractArray{K,N}, border::AbstractString, value)
    if border == "circular" && size(img) == size(kern)
        out = real(ifftshift(ifft(fft(img).*fft(kern))))
    elseif border != "inner"
        prepad  = [div(size(kern,i)-1, 2) for i = 1:N]
        postpad = [div(size(kern,i),   2) for i = 1:N]
        fullpad = [nextprod([2,3], size(img,i) + prepad[i] + postpad[i]) - size(img, i) - prepad[i] for i = 1:N]
        A = padarray(img, prepad, fullpad, border, convert(T, value))
        krn = zeros(typeof(one(T)*one(K)), size(A))
        indexesK = ntuple(d->[size(krn,d)-prepad[d]+1:size(krn,d);1:size(kern,d)-prepad[d]], N)::NTuple{N,Vector{Int}}
        AF = ifft(fft(A).*fft(krn))
        out = Array(realtype(eltype(AF)), size(img))
    end
    out
end

Test:

julia> @code_warntype ifi(rand(3,3), rand(3,3), "replicate", 0)
Variables:
  #self#::#ifi
  img::Array{Float64,2}
  kern::Array{Float64,2}
  border::ASCIIString
  value::Int64
  prepad::Box
...

Now comment out the indexesK = ... line (the output of which is not used at all). Suddenly prepad is inferred as Array{Int, 1}.

@JeffBezanson

This comment has been minimized.

Copy link
Member

JeffBezanson commented Feb 28, 2016

This is due to that line capturing prepad in a closure.

@andreasnoack

This comment has been minimized.

Copy link
Member

andreasnoack commented Mar 17, 2016

Just want to add that this has quite severe consequences for @threads for. Is it the plan to change/improve the behavior of Box or is it advised not to capture arrays in clusures? The last option will make it difficult for @threads for.

@KristofferC

This comment has been minimized.

Copy link
Contributor

KristofferC commented Mar 19, 2016

This also has severe consequences for NLsolve (JuliaNLSolvers/NLsolve.jl#49). Previously things were inferred fine:

image

but now everything is just Box and Any and performance is pretty much garbage..

image

Now sure how to work around this.

This feels like a quite significant regression. Now suddenly any variable captured by a closure poisons the variable for the whole outer function. Even if it is only read from.

Edit: The example below has been fixed.

Ex:

function newton_{T}(initial_x::Vector{T})

    nn = length(initial_x)
    xold = fill(convert(T, NaN), nn)
    fvec = Array(T, nn)
    f_calls = 1

    function fo(xlin::Vector)
        if xlin != xold
            print(f_calls)
        end
        return(dot(fvec, fvec) / 2)
    end

    return
end

@code_warntype newton_(rand(5))

Here, xold, f_calls and fvec are Box for the whole function. This used to work fine on 0.4.

nalimilan added a commit to nalimilan/FreqTables.jl that referenced this issue Apr 22, 2016

Fix type instability
Until the number of elements passed via varargs is inferred,
we need to use a wrapper function. Also remove access to lev[i]
from anonymous function, which is currently buggy on 0.5
(JuliaLang/julia#15276).

nalimilan added a commit to nalimilan/FreqTables.jl that referenced this issue Apr 22, 2016

Fix type instability
Until the number of elements passed via varargs is inferred,
we need to use a wrapper function. Also remove access to lev[i]
from anonymous function, which is currently buggy on 0.5
(JuliaLang/julia#15276).
@davidanthoff

This comment has been minimized.

Copy link
Contributor

davidanthoff commented Apr 25, 2016

Should this get a 0.5 label? For my projects this seems to cause a significant increase in runtime (~30%) for projects that worked well on 0.4.

@JeffBezanson

This comment has been minimized.

Copy link
Member

JeffBezanson commented Apr 26, 2016

Everything tagged "regression" will get attention before 0.5-final.

@JeffBezanson JeffBezanson self-assigned this Apr 26, 2016

@davidanthoff

This comment has been minimized.

Copy link
Contributor

davidanthoff commented Apr 26, 2016

Thanks!

JeffBezanson added a commit that referenced this issue Apr 26, 2016

@JeffBezanson

This comment has been minimized.

Copy link
Member

JeffBezanson commented Apr 26, 2016

Ok, I have a quick fix for many of the examples posted here. Not including @timholy 's original example in this issue, of course :/

@davidanthoff

This comment has been minimized.

Copy link
Contributor

davidanthoff commented Apr 26, 2016

I'll try my cases once this has landed in a windows nightly binary.

@JeffBezanson

This comment has been minimized.

Copy link
Member

JeffBezanson commented Apr 26, 2016

#16048 should be good now, #16050 not yet but also should be easy.

JeffBezanson added a commit that referenced this issue Apr 26, 2016

improve handling of local function bindings
Before they were pessimized a bit in a way that's no longer necessary
after the function redesign.

helps #15276

JeffBezanson added a commit that referenced this issue Apr 26, 2016

improve handling of local function bindings
Before they were pessimized a bit in a way that's no longer necessary
after the function redesign.

helps #15276
@mauro3

This comment has been minimized.

Copy link
Contributor

mauro3 commented Sep 26, 2018

The example in #15276 (comment) is still broken.

@Keno

This comment has been minimized.

Copy link
Member

Keno commented Dec 28, 2018

Note to self (or whoever ends up working on this). It'd be useful to also just directly emit an error if a value is captured and used but never assigned anywhere.

oyamad added a commit to oyamad/LRSLib.jl that referenced this issue Jan 3, 2019

oyamad added a commit to oyamad/LRSLib.jl that referenced this issue Jan 7, 2019

oyamad added a commit to oyamad/LRSLib.jl that referenced this issue Jan 9, 2019

oyamad added a commit to oyamad/LRSLib.jl that referenced this issue Jan 10, 2019

oyamad added a commit to oyamad/LRSLib.jl that referenced this issue Jan 10, 2019

@Nosferican

This comment has been minimized.

Copy link
Contributor

Nosferican commented Jan 19, 2019

From an user perspective, on Julia 1, is the recommended solution to use https://github.com/c42f/FastClosures.jl?

@StefanKarpinski

This comment has been minimized.

Copy link
Member

StefanKarpinski commented Jan 20, 2019

It's a solution. Another solution is to use let as necessary.

@c42f

This comment has been minimized.

Copy link
Contributor

c42f commented Jan 20, 2019

Yeah, it's hard to "recommend" something as ugly as FastClosures. But it does work.

oyamad added a commit to oyamad/LRSLib.jl that referenced this issue Jan 21, 2019

@chethega

This comment has been minimized.

Copy link
Contributor

chethega commented Jan 21, 2019

Re using let: It is somewhat unintuitive when let is helpful and when not. Especially the difference between let a = b and let\n a=b is unclear, in the case where a does not exist in the outer scope. Afaik there should be no difference at all (in the case where a does not exist in the outer scope).

I think we could make most closures fast if the critical decision (to box or not to box) could be deferred a little. It looks like this decision is made in the front-end, and the front-end does not understand the CFG.

We had an example on discourse, that I reproduce here:

function sieve(n::Int)
    prime = trues(n)
    rr=2:n
    it = iterate(rr)
    @label loopstart
    (it === nothing) && @goto loopend
    let 
        (i,s) = it #in order to make it fast delete the newline, i.e. "let (i,s) = it" 
        if any(i%j==0 for j in 2:i-1)
            prime[i]=false
        end
        it = iterate(rr,s)
        @goto loopstart
    end
    @label loopend
    return prime
end

I would have thought that both produce identical lowered code: let only disambiguates names/bindings.

This example is relevant because looping constructs introduce local scope. If we could fix this example, then we would probably automatically fix the natural spelling of this function (which should lower identically):

function sieve2(n::Int)
    prime = trues(n)
    for i in 2:n
        if any(i%j==0 for j in 2:i-1)
            prime[i]=false
        end
    end
    return prime
end
@dominikkiese

This comment has been minimized.

Copy link

dominikkiese commented Feb 1, 2019

I dont quite get the workaround offered by FastClosures.jl. If i have say a for loop annotated like Threads.@threads for how do you now correctly use the @closure macro? Sorry if this is already documented somehwere.

@c42f

This comment has been minimized.

Copy link
Contributor

c42f commented Feb 1, 2019

@dominikkiese @closure is not really composable in that way (that is, not composable with other macros which generate closures as part of their operation) It's just an ugly workaround; the real fix needs to be in the compiler. (We could possibly extend the ugly workaround to cover more cases possibly including @threads, but if you feel like doing that, please move this conversation over to the FastClosures issue tracker.)

@tkoolen

This comment has been minimized.

Copy link
Contributor

tkoolen commented Feb 1, 2019

Manually using let blocks as in #15276 (comment) still tends to work with @threads though.

@dominikkiese

This comment has been minimized.

Copy link

dominikkiese commented Feb 2, 2019

@tkoolen In that case what has to be included in the let block? All variables that are used inside the threaded for loop? What if the function called in that for loop itself has closures defined in it? Do they also need a let block? Is there actually any time frame for the issue to be settled in the compiler? It seems like the problem has been around for a long time but the fix apparently isn't easy at all.

JeffBezanson added a commit that referenced this issue Feb 5, 2019

JeffBezanson added a commit that referenced this issue Feb 6, 2019

JeffBezanson added a commit that referenced this issue Feb 7, 2019

@findmyway findmyway referenced this issue Feb 13, 2019

Open

Random Thoughts on v0.3.0 #24

0 of 3 tasks complete

@vchuravy vchuravy referenced this issue Feb 16, 2019

Draft

Experimental Tapir support #31086

2 of 8 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.