Add inlined methods for ntuple(f, Val{N}) for 0 ≤ N ≤ 15 #21446
Conversation
|
@nanosoldier |
ararslan
added
collections
|
Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @jrevels |
|
Should this go up to |
|
Might as well include the empty case |
Hmm... out of curiosity, does anyone know what happened to |
|
@andyferris They've been moved to a struct; see #18496. |
|
Cool - thank you very much! :) |
wsshin
changed the title
base/tuple.jl
Outdated
| @@ -123,6 +123,23 @@ end | |||
| _ntuple(f, n) = (@_noinline_meta; ([f(i) for i = 1:n]...)) | |||
|
|
|||
| # inferrable ntuple | |||
| ntuple{F}(f::F, ::Type{Val{0}}) = (@_inline_meta; ()) | |||
There was a problem hiding this comment.
we should also aim to use the new syntax
ntuple(f:F, ....) where {T}
There was a problem hiding this comment.
True, but I think that change must be applied to the entire code at once later.
There was a problem hiding this comment.
not necessarily, we have had PRs to gradually update the syntax ;)
There was a problem hiding this comment.
Cheers re. using where syntax. (Though IIRC, method specialization on function arguments happens automagically when the function arguments are called directly in the method's body, so chances are type parameter F is unnecessary.) Best!
There was a problem hiding this comment.
@Sacha0, interesting. So, did #20282 (comment) happen because the function argument was not called inside the method body?
|
Added the cases for N = 0 and 11 ≤ N ≤ 15. Wanted to do this without explicitly writing out those 16 definitions, but couldn't figure out a good way to automatically generate them. Any suggestions? |
|
How about this for N = 0:15
tup = Expr(:tuple)
tup.args = [Expr(:call, :f, i) for i = 1:N]
@eval ntuple{F}(f::F, ::Type{Val{$N}}) = (@_inline_meta; $tup)
end |
|
@pabloferz Can we change that |
|
@pabloferz, I actually tried something similar, but it seems that @andyferris, I also wanted to get |
|
@pabloferz, FYI, the following is the error I get during build procedure whenever I try to use |
|
You can get access to an instance of params = Core.Inference.InferenceParams(typemax(UInt))Instead of |
|
@vchuravy, thanks for the comments! Unfortunately, replacing If I simply put inside for N = 1:15
endwithout a loop body, then I get exactly the same build error starting with If I replace the N = 0
while N ≤ 15
N += 1
endthen I get a similar error starting with So, it seems that looping outside function definition is not allowed during bootstrap. Any workaround for this? |
|
This early in bootstrap, neither |
|
The problem is not let N = 0
while sle_int(N, 15)
tup = Expr(:tuple)
tup.args = Array{Any,1}(N)
i = 1
while sle_int(i, N)
Core.arrayset(tup.args, Expr(:call, :f, i), i)
i = add_int(i, 1)
end
@eval ntuple{F}(f::F, ::Type{Val{$N}}) = (@_inline_meta; $tup)
N = add_int(N, 1)
end
end@andyferris As things are now, is not possible. We could restore the global constants |
|
Since this code is not needed for the rest of bootstrap, it would be better to simply move it to a file later in the bootstrap process. |
|
As @TotalVerb said, other option would be moving this to another file even if its weird to have tuple related functions outside A third option would be to have the above definition take effect after if isdefined(Main, :Base)
N = 0
while sle_int(N, 15)
tup = Expr(:tuple)
tup.args = Array{Any,1}(N)
i = 1
while sle_int(i, N)
Core.arrayset(tup.args, Expr(:call, :f, i), i)
i = add_int(i, 1)
end
@eval ntuple{F}(f::F, ::Type{Val{$N}}) = (@_inline_meta; $tup)
N = add_int(N, 1)
end
end |
|
Perhaps the simplest and clearest solution would be to leave the definitions written out explicitly as they are now, and ignore the slight inelegance? :) |
|
Thank you all for wonderful suggestions! Personally I would like to follow @Sacha0's opinion.
So, I will push the version following @Sacha0's suggestion. I hope other suggestions suggested here to be considered in the future versions of |
|
@nanosoldier |
|
Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @jrevels |
|
@nanosoldier |
|
Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @jrevels |
|
Not sure why the change cause the performance regressions. The only change was the addition of the following lines: ntuple(f, ::Type{Val{0}}) = (@_inline_meta; ())
ntuple(f, ::Type{Val{1}}) = (@_inline_meta; (f(1),))
ntuple(f, ::Type{Val{2}}) = (@_inline_meta; (f(1), f(2)))
ntuple(f, ::Type{Val{3}}) = (@_inline_meta; (f(1), f(2), f(3)))
⋮
ntuple(f, ::Type{Val{15}}) = (@_inline_meta; (f(1), f(2), f(3), f(4), f(5), f(6), f(7), f(8), f(9), f(10), f(11), f(12), f(13), f(14), f(15)))The tests regressed are the following, and they are either irrelevant to tuples, or a too small test that is easily affected by random fluctuation:
|
|
@nanosoldier |
|
Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @jrevels |
|
The most recent nanosoldier results seem suspect (and apart from which all potential regressions so far look like noise), so let's ask nanosoldier again: @nanosoldier |
|
Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @jrevels |
|
The minor |
This PR adds
so that the calls to
ntuple(f, Val{N})is inlined for small N's satisfying 1 ≤ N ≤ 10.