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 regression from commit a2acf88 #6415

Closed
jtravs opened this issue Apr 4, 2014 · 6 comments
Closed

Performance regression from commit a2acf88 #6415

jtravs opened this issue Apr 4, 2014 · 6 comments
Labels
performance Must go faster regression Regression in behavior compared to a previous version

Comments

@jtravs
Copy link
Contributor

jtravs commented Apr 4, 2014

A code for quintic spline interpolation (which I call very often in a compute kernel, so needs to be fast) received a relatively significant (20%) performance regression after commit a2acf88

The code is at: https://gist.github.com/jtravs/9980928

With a2acf88
I get:
elapsed time: 0.006051846 seconds (6616 bytes allocated)
elapsed time: 0.006182501 seconds (48 bytes allocated)
elapsed time: 0.006065417 seconds (48 bytes allocated)
elapsed time: 0.002557683 seconds (48 bytes allocated)
elapsed time: 0.002540566 seconds (48 bytes allocated)
elapsed time: 0.002532169 seconds (48 bytes allocated)

With d19560d
I get:
elapsed time: 0.005068456 seconds (6700 bytes allocated)
elapsed time: 0.004971783 seconds (48 bytes allocated)
elapsed time: 0.00503566 seconds (48 bytes allocated)
elapsed time: 0.002130791 seconds (48 bytes allocated)
elapsed time: 0.002169476 seconds (48 bytes allocated)
elapsed time: 0.002222981 seconds (48 bytes allocated)

Any ideas why? I believe that commit was the inlining stuff.

@jtravs jtravs changed the title Performance regression from commit Performance regression from commit a2acf88 Apr 4, 2014
@JeffBezanson
Copy link
Member

I think I've got it. We are now pulling more values into temporary variables, the array-valued ones of which are given unnecessary GC roots. The old generated code was able to remove the GC frame entirely.

@JeffBezanson
Copy link
Member

A relevant example is qeval(quint{Float64}, Float64).

@JeffBezanson
Copy link
Member

A workaround for the time being is to make quint immutable.

@vtjnash any ideas? Moving x.a from argument position to an assignment RHS seems to require a GC root if x is mutable. Example code from qeval:

        _var7 = top(getfield)(q::quint{Float64},:F)::Array{Float64,1}
        _var8 = arrayref(_var7::Array{Float64,1},I::Int64)::Float64
        _var6 = top(getfield)(q::quint{Float64},:E)::Array{Float64,1}
        _var9 = arrayref(_var6::Array{Float64,1},I::Int64)::Float64
        _var5 = top(getfield)(q::quint{Float64},:D)::Array{Float64,1}
        _var10 = arrayref(_var5::Array{Float64,1},I::Int64)::Float64
        _var4 = top(getfield)(q::quint{Float64},:C)::Array{Float64,1}
        _var11 = arrayref(_var4::Array{Float64,1},I::Int64)::Float64
        _var3 = top(getfield)(q::quint{Float64},:B)::Array{Float64,1}
        _var12 = arrayref(_var3::Array{Float64,1},I::Int64)::Float64
        _var2 = top(getfield)(q::quint{Float64},:Y)::Array{Float64,1}
        _var13 = arrayref(_var2::Array{Float64,1},I::Int64)::Float64
        return top(box)(Float64,top(add_float)(top(box)(Float64,top(mul_float)(top(box)(Float64,top(add_float)(top(box)(Float64,top(mul_float)(top(box)(Float64,top(add_float)(top(box)(Float64,top(mul_float)(top(box)(Float64,top(add_float)(top(box)(Float64,top(mul_float)(top(box)(Float64,top(add_float)(top(box)(Float64,top(mul_float)(_var8::Float64,P::Float64))::Float64,_var9::Float64))::Float64,P::Float64))::Float64,_var10::Float64))::Float64,P::Float64))::Float64,_var11::Float64))::Float64,P::Float64))::Float64,_var12::Float64))::Float64,P::Float64))::Float64,_var13::Float64))::Float64

@vtjnash
Copy link
Member

vtjnash commented Apr 4, 2014

the effect_free test needs to pull out the call to arrayref to ensure that the order of statement execution is not being altered, since it cannot see the enclosing_ast to ensure there are no statements which aren't effect_free between it's intended execution order before and after the inlining

getfield has the same issue, but it knows that an immutable type could not have been modified

@JeffBezanson
Copy link
Member

Since all the inlined functions here are trivial (scalar + and *), it seems like we could know nothing will be reordered. Very hard to solve in general, but perhaps we can at least handle these simple cases pretty easily?

@vtjnash
Copy link
Member

vtjnash commented Apr 5, 2014

yes, as i mentioned, just need replace effect_free with a method of checking that none of the previous expressions could have an effect (i'm already inlining arguments from right-to-left to help with this)

vtjnash added a commit that referenced this issue Apr 6, 2014
vtjnash added a commit that referenced this issue Apr 6, 2014
@vtjnash vtjnash closed this as completed in c309133 Apr 7, 2014
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

3 participants