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 in unsafe_getindex #14594

Closed
KristofferC opened this issue Jan 7, 2016 · 2 comments
Closed

Performance regression in unsafe_getindex #14594

KristofferC opened this issue Jan 7, 2016 · 2 comments
Labels
performance Must go faster regression Regression in behavior compared to a previous version

Comments

@KristofferC
Copy link
Member

I looked at https://github.com/jrevels/BaseBenchmarkReports/blob/master/c70ab26/c70ab26_vs_adffe19.md which is a test benchmark run that @jrevels ran that compares 0.4.2 with master. There are multiple regressions and I believe that most of them are from changes to unsafe_getindex, and more specifically, _checksize

For example, the benchmark:

function sumvector(A, n)
    s = zero(eltype(A)) + zero(eltype(A))
    nrows = size(A, 1)
    ncols = size(A, 2)
    r = rand(1:nrows, 5)
    for k = 1:n
        for i = 1:ncols
            val = Base.unsafe_getindex(A, r, i)
            s += first(val)
        end
    end
    s
end

Benchmark with A = rand(300, 500).

Master:

julia> @time sumvector(A, 1000);
  0.126164 seconds (2.00 M allocations: 83.924 MB, 6.93% gc time)

0.4.2:

julia> @time sumvector(A, 1000)
  0.034920 seconds (1.00 M allocations: 61.035 MB, 7.24% gc time)

Profiling shows that the extra time is spend in _checksize at

_checksize(A::AbstractArray, dim, I, J...) = (size(A, dim) == length(I) || throw_checksize_error(A, dim, I); _checksize(A, dim+1, J...))

CC: @mbauman since it was last touched in the scalar dimension drop change.

@tkelman tkelman added performance Must go faster regression Regression in behavior compared to a previous version labels Jan 7, 2016
@mbauman
Copy link
Member

mbauman commented Jan 7, 2016

That benchmark tracker is already proving to be invaluable. Thanks for making this issue, @KristofferC. I'll try to look into this when I have time.

mbauman added a commit to mbauman/julia that referenced this issue Jan 8, 2016
In JuliaLang#13612 I converted checksize from a generated function to a recursive lispy definition, but the methods are too complicated to be automatically inlined. Manually adding the inline annotation fixes this performance regression JuliaLang#14594. Master is now faster than 0.4.0 on most of the array perf tests.
@mbauman
Copy link
Member

mbauman commented Jan 12, 2016

Fixed by #14609 and superseded by #14651 for the remaining regressions.

@mbauman mbauman closed this as completed Jan 12, 2016
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