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

Make similar faster and safer #18107

Merged
merged 2 commits into from
Aug 19, 2016
Merged

Make similar faster and safer #18107

merged 2 commits into from
Aug 19, 2016

Conversation

timholy
Copy link
Member

@timholy timholy commented Aug 18, 2016

By converting the eltype argument into a ::Type{T} parameter, we avoid runtime method lookup.

Perhaps more importantly, the introduction of NeedsShaping makes similar safer, by preventing any possibility of an infinite recursion.

By converting the eltype argument into a ::Type{T} parameter, we avoid runtime method lookup.

Perhaps more importantly, the introduction of NeedsShaping makes `similar` safer, by preventing any possibility of an infinite recursion.
@timholy
Copy link
Member Author

timholy commented Aug 18, 2016

@nanosoldier runbenchmarks(ALL, vs = ":master")

@nanosoldier
Copy link
Collaborator

Something went wrong when running your job: RemoteException(2,CapturedException(ErrorException("type NanosoldierError is immutable"),Any[( in report at BenchmarkJob.jl:405,1),( in run at BenchmarkJob.jl:233,1),( in #503 at multi.jl:1410,1),( in run_work_thunk at multi.jl:996,1),( in macro expansion at multi.jl:1410 [inlined],1),( in #502 at event.jl:46,1)]))
cc @jrevels

@timholy
Copy link
Member Author

timholy commented Aug 18, 2016

Since nanosoldier failed, it's worth saying that locally this resulted in modest improvements (15-40%) for about a dozen benchmarks, and no (reproducible) regressions.

similar{T}(a::AbstractArray{T}, dims::Tuple) = similar(a, T, to_shape(dims))
similar{T}(a::AbstractArray{T}, dims::DimOrInd...) = similar(a, T, to_shape(dims))
similar{T}(a::AbstractArray, ::Type{T}, dims::DimOrInd...) = similar(a, T, to_shape(dims))
similar{T}(a::AbstractArray, ::Type{T}, dims::NeedsShaping) = similar(a, T, to_shape(dims))
Copy link
Contributor

Choose a reason for hiding this comment

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

is this now a MethodError or handled elsewhere for dims not in this new Union?

Copy link
Member Author

@timholy timholy Aug 18, 2016

Choose a reason for hiding this comment

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

Anything that's not ::Dims or ::NeedsShaping definitely has to be handled elsewhere. That's been the design all along (see CustomUnitRanges.jl), but it turns out that if you mess up and fail to "catch" the similar method then it resulted in a StackOverflowError. This can't do that anymore.

Copy link
Contributor

Choose a reason for hiding this comment

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

should this add a test_throws MethodError for such a case?

@timholy
Copy link
Member Author

timholy commented Aug 18, 2016

Hmm, somehow this got closed by @jrevels fixing nanosoldier.

@timholy timholy reopened this Aug 18, 2016
@timholy
Copy link
Member Author

timholy commented Aug 18, 2016

I'm canceling the CI; everything passed the first time.

@jrevels
Copy link
Member

jrevels commented Aug 18, 2016

Oops, sorry about that. Gotta be more careful with my use of fixes on GitHub...

@tkelman tkelman merged commit 66bacec into master Aug 19, 2016
@tkelman tkelman deleted the teh/similar branch August 19, 2016 12:20
tkelman pushed a commit that referenced this pull request Aug 20, 2016
* Make similar faster and safer

By converting the eltype argument into a ::Type{T} parameter, we avoid runtime method lookup.

Perhaps more importantly, the introduction of NeedsShaping makes `similar` safer, by preventing any possibility of an infinite recursion.

* Test that similar throws a MethodError for unsupported dims types

(cherry picked from commit 66bacec)
mfasi pushed a commit to mfasi/julia that referenced this pull request Sep 5, 2016
* Make similar faster and safer

By converting the eltype argument into a ::Type{T} parameter, we avoid runtime method lookup.

Perhaps more importantly, the introduction of NeedsShaping makes `similar` safer, by preventing any possibility of an infinite recursion.

* Test that similar throws a MethodError for unsupported dims types
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.

4 participants