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

Improve performance of Symbol concatenation #41992

Merged
merged 2 commits into from
Aug 26, 2021

Conversation

BioTurboNick
Copy link
Contributor

# current: Symbol(x...) = Symbol(string(x...))
@btime Symbol(:test, :me)
  111.024 ns (3 allocations: 176 bytes)

Symbol(x::Symbol, y::Symbol) = Symbol(string(x), string(y))
@btime Symbol(:test, :me)
  84.826 ns (3 allocations: 96 bytes) - 25% better in time, 45% better in memory space

This concatenation method is used a lot in Plots.jl and has a material impact on performance: JuliaPlots/Plots.jl#3763

Copy link
Member

@oscardssmith oscardssmith left a comment

Choose a reason for hiding this comment

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

Looks good.

@mbauman
Copy link
Member

mbauman commented Aug 24, 2021

Seems like we should be able to do even better by going deeper. I seem to have a slower system than you:

Before:

julia> @btime Symbol(:test, :me)
  152.419 ns (3 allocations: 160 bytes)
:testme
lower level hackery
julia> @eval Base begin
       @inline function __unsafe_string!(out, s::Symbol, offs::Integer)
           n = sizeof(s)
           GC.@preserve s out unsafe_copyto!(pointer(out, offs), unsafe_convert(Ptr{UInt8},s), n)
           return n
       end

       function string(a::Union{Char, String, SubString{String}, Symbol}...)
           n = 0
           for v in a
               if v isa Char
                   n += ncodeunits(v)
               else
                   n += sizeof(v)
               end
           end
           out = _string_n(n)
           offs = 1
           for v in a
               offs += __unsafe_string!(out, v, offs)
           end
           return out
       end
       end
string (generic function with 21 methods)

After:

julia> @btime Symbol(:test, :me)
  119.850 ns (1 allocation: 32 bytes)
:testme

Similar ratio in time improvement, but 2 fewer allocations, 1/5th the space, and hugely more versatile.

@BioTurboNick
Copy link
Contributor Author

Oh nice, so shall I change to the low-level hackery? I can add some tests to make sure nothing breaks.

@mbauman
Copy link
Member

mbauman commented Aug 25, 2021

I just pushed it — I already had it locally. It'd be good to ensure we have testing that includes heterogeneous combinations of a Symbol or two with chars and strings; if you could add that or verify it it'd be great!

@mbauman mbauman changed the title Improve performance of 2-arg Symbol concatenation Improve performance of Symbol concatenation Aug 25, 2021
@BioTurboNick
Copy link
Contributor Author

I took a look and I think the existing tests seem comprehensive enough?

@testset "Symbol and gensym" begin
    @test Symbol("asdf") === :asdf
    @test Symbol(:abc,"def",'g',"hi",0) === :abcdefghi0
    @test :a < :b
    @test startswith(string(gensym("asdf")),"##asdf#")
    @test gensym("asdf") != gensym("asdf")
    @test gensym() != gensym()
    @test startswith(string(gensym()),"##")
    @test_throws ArgumentError Symbol("ab\0")
    @test_throws ArgumentError gensym("ab\0")
end

Maybe the only thing useful to add would be empties?

@test Symbol("", :"") == :""
@test Symbol(:"", :"") == :""
@test Symbol(a, :"") == :a
@test Symbol(:"", a) == :a

@KristofferC KristofferC merged commit f1b5abc into JuliaLang:master Aug 26, 2021
@KristofferC KristofferC added the performance Must go faster label Aug 26, 2021
LilithHafner pushed a commit to LilithHafner/julia that referenced this pull request Feb 22, 2022
LilithHafner pushed a commit to LilithHafner/julia that referenced this pull request Mar 8, 2022
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 this pull request may close these issues.

6 participants