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

Update checks for nothing #964

Merged
merged 1 commit into from
Nov 18, 2019
Merged

Update checks for nothing #964

merged 1 commit into from
Nov 18, 2019

Conversation

devmotion
Copy link
Member

Using === nothing instead of == nothing can be slightly more performant (see JuliaLang/julia#29674 (comment)) and it doesn't hurt to use === consistently. Additionally, === always returns a Bool whereas e.g. missing == nothing evaluates to missing.

@mohamed82008
Copy link
Member

I don't see the perf issue with x == nothing. Especially with constant propagation, these if statements all compile away now:

julia> f(x) = x == nothing ? 1 : 2;

julia> g(x) = x === nothing ? 1 : 2;

julia> @code_typed f(1)
CodeInfo(
1 ─     goto #3 if not false
2nothing::Nothing
3return 2
) => Int64

julia> @code_typed g(1)
CodeInfo(
1 ─     goto #3 if not false
2nothing::Nothing
3return 2
) => Int64

@devmotion
Copy link
Member Author

As I wrote, it's not always more performant and in some cases constant propagation is sufficient. However, in general that's not true, and hence for consistency we changed all (in)equality checks with nothing (and other singleton types) to === and !== in JuliaDiffEq (see, e.g., SciML/OrdinaryDiffEq.jl#587). Moreover, using === or !== has the advantage that the expression is guaranteed to evaluate to true or false.

The original example from base (JuliaLang/julia#28695) is still significantly slower with regular equality checks on Julia 1.2:

using BenchmarkTools

function f(d)
   it = keys(d)
   res = 0
   elst1 = iterate(it)
   while elst1 != nothing
      s1,ns1 = elst1
      elst2 = iterate(it,ns1)
      while elst2 != nothing
         s2,ns2 = elst2
         res += d[s1]*d[s2]
         elst2 = iterate(it,ns2)
      end
      elst1 = iterate(it,ns1)
   end
   res
end

function g(d)
   it = keys(d)
   res = 0
   elst1 = iterate(it)
   while elst1 !== nothing
      s1,ns1 = elst1
      elst2 = iterate(it,ns1)
      while elst2 !== nothing
         s2,ns2 = elst2
         res += d[s1]*d[s2]
         elst2 = iterate(it,ns2)
      end
      elst1 = iterate(it,ns1)
   end
   res
end

d = Dict(:a=>1, :b=>2, :c=>3, :d=>4)

@btime f($d) # 218.605 ns (10 allocations: 320 bytes)
@btime g($d) # 159.806 ns (0 allocations: 0 bytes)

@mohamed82008
Copy link
Member

I see your point, but I think this example is a bit too special because it uses iterate which has an inferred return type Union{T, Nothing}, so constant propagation is not enough to compile the check away. I doubt this will be a problem for us in Turing since the way we used nothing is pretty basic. Could you please run a benchmark to see if any speedup results from this PR?

@trappmartin
Copy link
Member

Thanks!

I don't see an issue with merging this. Maybe we can simply run the MCMC benchmarks on this and see what happens.

Copy link
Member

@cpfiffer cpfiffer left a comment

Choose a reason for hiding this comment

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

I've got no issues with merging this one.

@yebai yebai merged commit 955f6c1 into TuringLang:master Nov 18, 2019
@devmotion devmotion deleted the nothing branch November 18, 2019 17:56
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.

5 participants