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

Loading CSV.jl makes conversion of a Some{Any} to a Some{Any} 200 times slower #31535

Closed
KristofferC opened this issue Mar 29, 2019 · 4 comments · Fixed by #31536
Closed

Loading CSV.jl makes conversion of a Some{Any} to a Some{Any} 200 times slower #31535

KristofferC opened this issue Mar 29, 2019 · 4 comments · Fixed by #31536
Labels
performance Must go faster

Comments

@KristofferC
Copy link
Sponsor Member

KristofferC commented Mar 29, 2019

julia> using BenchmarkTools

julia> c = Some{Any}("foo")
Some("foo")

julia> @code_warntype convert(Some{Any}, c)
Body::Some{Any}
1%1 = (Base.getfield)(x, :value)::Any%2 = %new(Some{Any}, %1)::Some{Any}
└──      return %2

julia> @btime convert(Some{Any}, $c)
  4.299 ns (1 allocation: 16 bytes)
Some("foo")

julia> using CSV

julia> @code_warntype convert(Some{Any}, c)
Body::Some{Any}
1%1 = $(Expr(:static_parameter, 1))::Core.Compiler.Const(Any, false)
│   %2 = (Base.getfield)(x, :value)::Any%3 = (Base.convert)(%1, %2)::Any%4 = (Some{Any})(%3)::Some{Any}
└──      return %4

julia> @btime convert(Some{Any}, $c)
  923.077 ns (1 allocation: 16 bytes)
Some("foo")

Noted in JuliaDebug/JuliaInterpreter.jl#206 (comment)

@timholy
Copy link
Sponsor Member

timholy commented Mar 29, 2019

On master it's slightly different but same outcome:

julia> c = Some{Any}("foo")
Some("foo")

julia> @code_warntype convert(Some{Any}, c)
Variables
  #self#::Core.Compiler.Const(convert, false)
  #unused#::Core.Compiler.Const(Some{Any}, false)
  x::Some{Any}

Body::Some{Any}
1%1 = Core.apply_type(Base.Some, $(Expr(:static_parameter, 1)))::Core.Compiler.Const(Some{Any}, false)
│   %2 = $(Expr(:static_parameter, 1))::Core.Compiler.Const(Any, false)
│   %3 = Base.getproperty(x, :value)::Any%4 = Base.convert(%2, %3)::Any%5 = (%1)(%4)::Some{Any}
└──      return %5

julia> m = @which convert(Some{Any}, c)
convert(::Type{Some{T}}, x::Some) where T in Base at some.jl:18

julia> m.specializations
Core.TypeMapEntry(nothing, Tuple{typeof(convert),Type{Some{Any}},Some{Any}}, nothing, svec(), 25990, -1, MethodInstance for convert(::Type{Some{Any}}, ::Some{Any}), false, true, false)

julia> using BenchmarkTools

julia> @btime convert(Some{Any}, $c)
  3.656 ns (1 allocation: 16 bytes)
Some("foo")

julia> length(methods(Base.getproperty))
21

julia> length(methods(Base.convert))
187

julia> using CSV

julia> @code_warntype convert(Some{Any}, c)
Variables
  #self#::Core.Compiler.Const(convert, false)
  #unused#::Core.Compiler.Const(Some{Any}, false)
  x::Some{Any}

Body::Some{Any}
1%1 = Core.apply_type(Base.Some, $(Expr(:static_parameter, 1)))::Core.Compiler.Const(Some{Any}, false)
│   %2 = $(Expr(:static_parameter, 1))::Core.Compiler.Const(Any, false)
│   %3 = Base.getproperty(x, :value)::Any%4 = Base.convert(%2, %3)::Any%5 = (%1)(%4)::Some{Any}
└──      return %5

julia> m == @which convert(Some{Any}, c)
true

julia> m.specializations
Core.TypeMapEntry(Core.TypeMapEntry(nothing, Tuple{typeof(convert),Type{Some{Any}},Some{Any}}, nothing, svec(), 8030, 26005, MethodInstance for convert(::Type{Some{Any}}, ::Some{Any}), false, true, false), Tuple{typeof(convert),Type{Some{Any}},Some{Any}}, nothing, svec(), 26027, -1, MethodInstance for convert(::Type{Some{Any}}, ::Some{Any}), false, true, false)

julia> @btime convert(Some{Any}, $c)
  1.001 μs (1 allocation: 16 bytes)
Some("foo")

julia> length(methods(Base.getproperty))
49

julia> length(methods(Base.convert))
240

Profiling reveals it's using dynamic dispatch after loading CSV, and wasn't before. I'm not sure if there's a general fix, it may just require #31536.

@JeffBezanson
Copy link
Sponsor Member

Keep in mind code_warntype is showing the code without inlining.

My first thought was that this could be due to CategoricalArrays adding another method for convert(Any, ...). The version of it I have installed removed that method, and indeed I don't see this timing difference. Could you check your version?

@vtjnash
Copy link
Sponsor Member

vtjnash commented Mar 29, 2019

There looks to be some type-piracy introduced by CategoricalArrays:

julia> methods(convert, (Type{Any}, Any))
[1] convert(::Type{Any}, x::Union{CategoricalArrays.CategoricalString{R}, CategoricalArrays.CategoricalValue{T,R} where T} where R) in CategoricalArrays at /Users/jameson/.julia/packages/CategoricalArrays/ucKV2/src/value.jl:83
[2] convert(::Type{Any}, x) in Base at essentials.jl:166

@JeffBezanson
Copy link
Sponsor Member

Jinx!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance Must go faster
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants