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

Ensure checksize inlines #14609

Merged
merged 1 commit into from
Jan 11, 2016
Merged

Conversation

mbauman
Copy link
Sponsor Member

@mbauman mbauman commented Jan 8, 2016

In #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 #14594. Master is now faster than 0.4.0 on most of the array perf tests.

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
Sponsor Member Author

mbauman commented Jan 8, 2016

runbenchmarks("arrays", vs = "JuliaLang/julia:master")

On my computer, I still see a few perf regressions from 0.4.0:

More than 2x perf regression:

sumrangeIb BitArray
sumcolonFb ArrayLF
sumcolonFb ArrayLS
sumcolonFb ArrayLSLS
sumrangeFb BitArray

More than 1.2x perf regression:

sumcolonIb ArrayLS
sumcolonIb ArrayLSLS
sumrangeIb ArrayLSLS
sumcolonIb BitArray
sumcolonFb Array
sumrangeFb ArrayLF
sumrangeFb ArrayLS
sumrangeFb ArrayLSLS
sumcolonFb BitArray

@@ -247,18 +247,25 @@ end
end

# checksize ensures the output array A is the correct size for the given indices
@noinline throw_checksize_error(arr, dim, idx) = throw(DimensionMismatch("index $d selects $(length(I[d])) elements, but size(A, $d) = $(size(A,d))"))
@noinline throw_checksize_error(A, dim, idx) = throw(DimensionMismatch("index $dim selects $(length(idx)) elements, but size(A, $dim) = $(size(A,dim))"))
@noinline throw_checksize_error(A, dim, idx::AbstractArray{Bool}) = throw(DimensionMismatch("index $dim selects $(sum(idx)) elements, but size(A, $dim) = $(size(A,dim))"))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isn't the size of the boolean array more important than how many elements are true?

Copy link
Sponsor Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is checking the size of the output array… it's essentially ensuring that similar returned an object of the correct size. It's of dubious value (looks like nobody hit the error since I had also introduced a bug in the error message), but it's cheap when done correctly. IIRC, it's what the builtin array types did before #10525, but my memory there is fuzzy.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh my mistake, I thought it was checking the size of the input. I should read the comments. Too used to there not being any comments to read, or something.

@KristofferC
Copy link
Sponsor Member

Maybe you could run a benchmark against julia 0.4.2 as well?

@KristofferC
Copy link
Sponsor Member

Regarding the regression in sumcolonFb ArrayLF

On master there is a significant time spent in _unsafe_getindex in array.jl and cartesian.jl while in 0.4.2 there is no time spent in those files.

I'm having a hard time digging through the generated functions coupled with the Cartesian macros so can't really pinpoint exactly where it happens..

@KristofferC
Copy link
Sponsor Member

Hmm, regarding my last comment, it could also be that we just have better inlining debug info for the profiler.

@jrevels
Copy link
Member

jrevels commented Jan 9, 2016

The GC "regressions" that @nanosoldier detected here are erroneous, see JuliaCI/BenchmarkTrackers.jl#5. The raw time improvements are great!

mbauman added a commit that referenced this pull request Jan 11, 2016
@mbauman mbauman merged commit 2335ef1 into JuliaLang:master Jan 11, 2016
@mbauman mbauman deleted the mb/inline-checksize branch January 11, 2016 18:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants