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

Dot operation between a tuple and a scalar variable generates type instability #21291

Closed
wsshin opened this issue Apr 5, 2017 · 19 comments
Closed
Labels
broadcast Applying a function over a collection

Comments

@wsshin
Copy link
Contributor

wsshin commented Apr 5, 2017

For example, when a constant Int64 is dot-added to a Tuple{Int64,Int64,Int64}, the result correctly remains a Tuple{Int64,Int64,Int64}:

julia> VERSION
v"0.6.0-pre.beta.30"

julia> function dotadd_tuple_intc()
           t = (1,0,1)
           t = t .+ 1
       end
dotadd_tuple_intc (generic function with 1 method)

julia> @code_warntype dotadd_tuple_intc()
Variables:
  #self#::#dotadd_tuple_intc
  t::Tuple{Int64,Int64,Int64}
  #41::##41#42

However, if you assign the constant Int64 to a variable and dot-add that variable to a Tuple{Int64,Int64,Int64}, the result is no longer a Tuple{Int64,Int64,Int64}, but just an abstract Tuple:

julia> function dotadd_tuple_intv()
           o = 1
           t = (1,0,1)
           t = t .+ o
       end
dotadd_tuple_intv (generic function with 1 method)

julia> @code_warntype dotadd_tuple_intv()
Variables:
  #self#::#dotadd_tuple_intv
  o::Int64
  t::Tuple
  #3::Base.Broadcast.##3#4{Base.#+,Tuple{Tuple{Int64,Int64,Int64},Int64}}

If we directly use broadcast, the same type instability occurs even if we do not assign the constant Int64 to a variable :

julia> function bcadd_tuple_intc()
           t = (1,0,1)
           t = broadcast(+, t, 1)
       end
bcadd_tuple_intc (generic function with 1 method)

julia> @code_warntype bcadd_tuple_intc()
Variables:
  #self#::#bcadd_tuple_intc
  t::Tuple
  #3::Base.Broadcast.##3#4{Base.#+,Tuple{Tuple{Int64,Int64,Int64},Int64}}
@stevengj stevengj added the broadcast Applying a function over a collection label Apr 6, 2017
@stevengj
Copy link
Member

stevengj commented Apr 6, 2017

The literal constant in t .+ 1 is inlined, so type inference works better for it (and it is faster). But it would be good to get type inference working for the general case too.

Note that type inference works fine for the array case t = [1,0,1].

cc @pabloferz

@martinholters
Copy link
Member

Also broadcast(+, t, (1,1,1)) works fine, but for the mixed tuple/scalar case, the result size is handed to ntuple as a runtime parameter (i.e. n, not Val{n}).

@pabloferz
Copy link
Contributor

I have a half-baked solution for this. Will submit a PR soon.

@pabloferz
Copy link
Contributor

Closed by #21331

@wsshin
Copy link
Contributor Author

wsshin commented Apr 11, 2017

Thanks! This fix helps a lot.

One more question. With this fix, dot-addition between a tuple and a scalar works not much slower than dot-addition between a StaticArray and a scalar:

julia> VERSION
v"0.6.0-pre.beta.86"

julia> using BenchmarkTools

julia> using StaticArrays

julia> function dotadd_tuple_intv()
           o = 1
           t = (1,0,1)
           t = t .+ o
       end
dotadd_tuple_intv (generic function with 1 method)

julia> function dotadd_svec_intv()
           o = 1
           v = @SVector([1,0,1])
           v = v .+ o
       end
dotadd_svec_intv (generic function with 1 method)

julia> @btime dotadd_tuple_intv()
  10.647 ns (0 allocations: 0 bytes)

julia> @btime dotadd_svec_intv()
  7.007 ns (0 allocations: 0 bytes)

However, inlining makes the StaticArray version much faster than the tuple version:

julia> @inline function inlined_dotadd_tuple_intv()
           o = 1
           t = (1,0,1)
           t = t .+ o
       end
inlined_dotadd_tuple_intv (generic function with 1 method)

julia> @inline function inlined_dotadd_svec_intv()
           o = 1
           v = @SVector([1,0,1])
           v = v .+ o
       end
inlined_dotadd_svec_intv (generic function with 1 method)

julia> @btime inlined_dotadd_tuple_intv()
  8.219 ns (0 allocations: 0 bytes)

julia> @btime inlined_dotadd_svec_intv()
  1.894 ns (0 allocations: 0 bytes)

I wonder if there is a way to make the inlined function with tuple operations as fast as the inlined function with StaticArray operations.

@pabloferz
Copy link
Contributor

pabloferz commented Apr 11, 2017

Hi @wsshin. You seem to be benchmarking in global scope, if you define

f() = inlined_dotadd_tuple_intv()
g() = inlined_dotadd_svec_intv()

you should observe the same difference you had with dotadd_tuple_intv and dotadd_svec_intv. Cheers!

@wsshin
Copy link
Contributor Author

wsshin commented Apr 11, 2017

I see. I knew functions defined in REPL are defined in global scope, but I though (without knowing why) I could benchmark functions directly if they do not take any arguments. (Of course, when benchmarking functions with arguments, I interpolated the arguments with $.)

Why does this convention not work here? Is this because of the @inline macro?

@yuyichao
Copy link
Contributor

There doesn't seem to be any "Benchmark in global scope" here.

@wsshin
Copy link
Contributor Author

wsshin commented Apr 11, 2017

@yuyichao, do you mean it is incorrect to say that I was benchmarking in global scope? Then why is wrapping by another function (f and g in @pabloferz's example) the correct way of benchmarking an inlined function?

@yuyichao
Copy link
Contributor

do you mean it is incorrect to say that I was benchmarking in global scope?

Right.

Then why is wrapping by another function (f and g in @pabloferz's example) the correct way of benchmarking an inlined function?

No it's not. It just disabled inlining.

@wsshin
Copy link
Contributor Author

wsshin commented Apr 11, 2017

@yuyichao, thanks for your answer!

@pabloferz, then back to my original question: is there a way to make an inlined function that uses tuple operations as fast as an inlined function that uses equivalent StaticArray operations?

@yuyichao
Copy link
Contributor

The issue seems to be that ntuple isn't inlined in the tuple version.

@pabloferz
Copy link
Contributor

Oh, right. Sorry, @yuyichao is right, ntuple (on which broadcast over tuples relies) is not inlined. In the StaticArrays case the whole broadcast method is most likely getting inlined.

@Sacha0
Copy link
Member

Sacha0 commented Apr 11, 2017

Hm, @inlineing ntuple(..., Val{N}) specifically might be reasonable? Thoughts @pabloferz?

@pabloferz
Copy link
Contributor

I guess it might be reasonable as long as we put some limit on the N to prevent inlining very long tuples.

@wsshin
Copy link
Contributor Author

wsshin commented Apr 12, 2017

@Sacha0, @pabloferz, is there a PR on this ntuple issue? I would like to follow it.

@Sacha0
Copy link
Member

Sacha0 commented Apr 12, 2017

is there a PR on this ntuple issue? I would like to follow it.

No PR exists specifically for inlining ntuple(..., Val{N}) insofar as I am aware. Simple change to make and test though --- would you like to have a go? :)

@wsshin
Copy link
Contributor Author

wsshin commented Apr 12, 2017

@Sacha0, sure. Let me look into this.

@wsshin
Copy link
Contributor Author

wsshin commented Apr 19, 2017

PR inlining ntuple(f, Val{N}) for 1 ≤ N ≤ 10 is created: #21446.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
broadcast Applying a function over a collection
Projects
None yet
Development

No branches or pull requests

6 participants