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

Reduce number of getindex(::Type, ...) methods #44127

Merged
merged 1 commit into from
Feb 18, 2022

Conversation

martinholters
Copy link
Member

Previously, there were special cases for T[], T[a], T[a,b] and T[a,b,c]. Together with the general case for more elements, that meant five methods to consider in cases like T[x...] where the length of x was not known at compile time. That was beyond the inference limit and such a call would be inferred as Any. So this change gets rid of all the special cases.

The loop-based general case worked well if all arguments were of the same type, but otherwise suffered from type-instability inside the loop. Without the special cases for low element count this would be hit more often, so the loop is replaced with a call to afoldl that basically unrolls the loop for up to 32 elements.

julia> f(x) = Int[x...];

julia> @code_typed(f([1]))[2]
Any # master
Vector{Int64} (alias for Array{Int64, 1}) # PR

julia> @btime Float64[1, 2.0, 0x3];
  35.326 ns (1 allocation: 80 bytes) # master
  35.215 ns (1 allocation: 80 bytes) # PR

julia> @btime Float64[1, 2.0, 0x3, 4.0f0];
  255.610 ns (7 allocations: 256 bytes) # master
  36.245 ns (1 allocation: 96 bytes) # PR

julia> @btime Float64[1, 2, 3, 4];
  35.578 ns (1 allocation: 96 bytes) # master
  35.143 ns (1 allocation: 96 bytes) # PR

Motivated by JuliaMath/FFTW.jl#231.

base/array.jl Outdated Show resolved Hide resolved
@JeffBezanson
Copy link
Member

JeffBezanson commented Feb 11, 2022

This is good; I think the only downside is that we lose the vararg tuple elision after 32:

julia> @btime Int[1,2,3,4,5,6,7,8,9,10,11,12,13,14,15,16,17,18,19,20,21,22,23,24,25,26,27,28,29,30,31,32,33]
  32.142 ns (1 allocation: 336 bytes)

julia> @btime gi(Int,1,2,3,4,5,6,7,8,9,10,11,12,13,14,15,16,17,18,19,20,21,22,23,24,25,26,27,28,29,30,31,32,33)
  351.113 ns (4 allocations: 656 bytes)

(gi is the method from this PR)
Maybe manually inlining afoldl would fix it? Declaring the call site @inline doesn't seem to work; maybe it is too big.

@N5N3
Copy link
Member

N5N3 commented Feb 11, 2022

I guess we can use for loop if vals isa NTuple?
Similar to #44063, that PR also wants to resolve instability in loop.

@martinholters martinholters force-pushed the mh/getindex-type-with-afoldl branch from 71c4298 to b8bbc31 Compare February 14, 2022 13:30
@martinholters
Copy link
Member Author

martinholters commented Feb 15, 2022

Updated with @N5N3's idea to keep the for loop in the NTuple case. Now Int[1,2,3,4,5,6,7,8,9,10,11,12,13,14,15,16,17,18,19,20,21,22,23,24,25,26,27,28,29,30,31,32,33] looks good (=as in master), too. CI failure on linux64 is a timeout I'd assume to be unrelated.

@KristofferC KristofferC added the backport 1.8 Change should be backported to release-1.8 label Feb 16, 2022
Previously, there were special cases for `T[]`, `T[a]`, `T[a,b]` and
`T[a,b,c]`. Together with the general case for more elements, that meant
five methods to consider in cases like `T[x...]` where the length of `x`
was not known at compile time. That was beyond the inference limit and
such a call would be inferred as `Any`. So this change gets rid of all
the special cases.

The loop-based general case worked well if all arguments were of the
same type, but otherwise suffered from type-instability inside the loop.
Without the special cases for low element count this would be hit more
often, so for the non-homogenous case, the loop is replaced with a call
to `afoldl` that basically unrolls the loop for up to 32 elements.
@martinholters martinholters force-pushed the mh/getindex-type-with-afoldl branch from b8bbc31 to f9db5d9 Compare February 16, 2022 13:40
Copy link
Member

@vtjnash vtjnash left a comment

Choose a reason for hiding this comment

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

SGTM. Seems a clever solution to this

@martinholters
Copy link
Member Author

CI is all green after another rebase, feedback is generally positive. I'm going to merge tomorrow unless anyone objects (or beats me to it).

@martinholters
Copy link
Member Author

CI is all green after another rebase

I spoke too soon, buildbot/tester_linux64 timed out again. But I've seen that in other PRs, too, so let me declare it unrelated.

@martinholters martinholters merged commit b8e5d7e into master Feb 18, 2022
@martinholters martinholters deleted the mh/getindex-type-with-afoldl branch February 18, 2022 08:47
@oscardssmith
Copy link
Member

Why is this getting backported?

@KristofferC
Copy link
Member

There is some leeway for PRs that were more or less finished when feature freeze hit and just needed review/CI to go through.

KristofferC pushed a commit that referenced this pull request Feb 19, 2022
Previously, there were special cases for `T[]`, `T[a]`, `T[a,b]` and
`T[a,b,c]`. Together with the general case for more elements, that meant
five methods to consider in cases like `T[x...]` where the length of `x`
was not known at compile time. That was beyond the inference limit and
such a call would be inferred as `Any`. So this change gets rid of all
the special cases.

The loop-based general case worked well if all arguments were of the
same type, but otherwise suffered from type-instability inside the loop.
Without the special cases for low element count this would be hit more
often, so for the non-homogeneous case, the loop is replaced with a call
to `afoldl` that basically unrolls the loop for up to 32 elements.

(cherry picked from commit b8e5d7e)
@KristofferC KristofferC removed the backport 1.8 Change should be backported to release-1.8 label Feb 24, 2022
staticfloat pushed a commit to JuliaCI/julia-buildkite-testing that referenced this pull request Mar 2, 2022
Previously, there were special cases for `T[]`, `T[a]`, `T[a,b]` and
`T[a,b,c]`. Together with the general case for more elements, that meant
five methods to consider in cases like `T[x...]` where the length of `x`
was not known at compile time. That was beyond the inference limit and
such a call would be inferred as `Any`. So this change gets rid of all
the special cases.

The loop-based general case worked well if all arguments were of the
same type, but otherwise suffered from type-instability inside the loop.
Without the special cases for low element count this would be hit more
often, so for the non-homogeneous case, the loop is replaced with a call
to `afoldl` that basically unrolls the loop for up to 32 elements.
LilithHafner pushed a commit to LilithHafner/julia that referenced this pull request Mar 8, 2022
Previously, there were special cases for `T[]`, `T[a]`, `T[a,b]` and
`T[a,b,c]`. Together with the general case for more elements, that meant
five methods to consider in cases like `T[x...]` where the length of `x`
was not known at compile time. That was beyond the inference limit and
such a call would be inferred as `Any`. So this change gets rid of all
the special cases.

The loop-based general case worked well if all arguments were of the
same type, but otherwise suffered from type-instability inside the loop.
Without the special cases for low element count this would be hit more
often, so for the non-homogeneous case, the loop is replaced with a call
to `afoldl` that basically unrolls the loop for up to 32 elements.
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.

7 participants