Skip to content

optimize Symbol with constant string argument #32437

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.

Sign up for GitHub

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

Merged
JeffBezanson merged 1 commit into from
Nov 30, 2019
Merged

optimize Symbol with constant string argument #32437

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions base/boot.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -432,11 +432,12 @@ Array{T}(A::AbstractArray{S,N}) where {T,N,S} = Array{T,N}(A)
AbstractArray{T}(A::AbstractArray{S,N}) where {T,S,N} = AbstractArray{T,N}(A)

# primitive Symbol constructors
function Symbol(s::String)
eval(Core, :(function Symbol(s::String)
$(Expr(:meta, :pure))
return ccall(:jl_symbol_n, Ref{Symbol}, (Ptr{UInt8}, Int),
ccall(:jl_string_ptr, Ptr{UInt8}, (Any,), s),
sizeof(s))
end
end))
function Symbol(a::Array{UInt8,1})
return ccall(:jl_symbol_n, Ref{Symbol}, (Ptr{UInt8}, Int),
ccall(:jl_array_ptr, Ptr{UInt8}, (Any,), a),
Expand Down
4 changes: 3 additions & 1 deletion base/compiler/utilities.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,9 @@ function quoted(@nospecialize(x))
end

function is_inlineable_constant(@nospecialize(x))
x isa Type && return true
if x isa Type || x isa Symbol
return true
end
return isbits(x) && Core.sizeof(x) <= MAX_INLINE_CONST_SIZE
end

Expand Down
6 changes: 3 additions & 3 deletions base/strings/string.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ Convert a string to a contiguous byte array representation encoded as UTF-8 byte
This representation is often appropriate for passing strings to C.
"""
String(s::AbstractString) = print_to_string(s)
String(s::Symbol) = unsafe_string(unsafe_convert(Ptr{UInt8}, s))
@pure String(s::Symbol) = unsafe_string(unsafe_convert(Ptr{UInt8}, s))

unsafe_wrap(::Type{Vector{UInt8}}, s::String) = ccall(:jl_string_to_array, Ref{Vector{UInt8}}, (Any,), s)

Expand All @@ -81,8 +81,8 @@ String(s::CodeUnits{UInt8,String}) = s.s
pointer(s::String) = unsafe_convert(Ptr{UInt8}, s)
pointer(s::String, i::Integer) = pointer(s)+(i-1)

ncodeunits(s::String) = Core.sizeof(s)
sizeof(s::String) = Core.sizeof(s)
@pure ncodeunits(s::String) = Core.sizeof(s)
@pure sizeof(s::String) = Core.sizeof(s)
Copy link
Member

@vtjnash vtjnash Jun 27, 2019

Choose a reason for hiding this comment

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

I think inference is supposed to know this one already, and might just need a minor fix

Copy link
Member Author

@JeffBezanson JeffBezanson Jun 27, 2019

Choose a reason for hiding this comment

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

Actually a while ago I specifically told the compiler not to constant prop Strings since it led to extremely long compile times for some packages (if I recall, DiffEq parsing many string literals to input numeric constants).

Copy link
Member

@Keno Keno Jul 2, 2019

Choose a reason for hiding this comment

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

purity detection should be able to infer this independently of constant prop though and mark it as pure

codeunit(s::String) = UInt8

@inline function codeunit(s::String, i::Integer)
Expand Down
17 changes: 10 additions & 7 deletions test/compiler/inference.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1711,20 +1711,19 @@ g26826(x) = getfield26826(x, :a, :b)
# If this test is broken (especially if inference is getting a correct, but loose result,
# like a Union) then it's potentially an indication that the optimizer isn't hitting the
# InferenceResult cache properly for varargs methods.
typed_code = Core.Compiler.code_typed(f26826, (Float64,))[1].first
found_well_typed_getfield_call = false
let i
let ct = Core.Compiler.code_typed(f26826, (Float64,))[1]
typed_code, retty = ct.first, ct.second
found_poorly_typed_getfield_call = false
for i = 1:length(typed_code.code)
stmt = typed_code.code[i]
rhs = Meta.isexpr(stmt, :(=)) ? stmt.args[2] : stmt
if Meta.isexpr(rhs, :call) && rhs.args[1] == GlobalRef(Base, :getfield) && typed_code.ssavaluetypes[i] === Float64
global found_well_typed_getfield_call = true
if Meta.isexpr(rhs, :call) && rhs.args[1] == GlobalRef(Base, :getfield) && typed_code.ssavaluetypes[i] !== Float64
found_poorly_typed_getfield_call = true
end
end
@test !found_poorly_typed_getfield_call && retty === Float64
end

@test found_well_typed_getfield_call

# 27059 fix fieldtype vararg and union handling

f27059(::Type{T}) where T = i -> fieldtype(T, i)
Expand Down Expand Up @@ -2482,3 +2481,7 @@ end
@test Base.return_types(g33768, ()) == Any[Any]
@test_throws ArgumentError h33768()
@test Base.return_types(h33768, ()) == Any[Union{}]

# constant prop of `Symbol("")`
f_getf_computed_symbol(p) = getfield(p, Symbol("first"))
@test Base.return_types(f_getf_computed_symbol, Tuple{Pair{Int8,String}}) == [Int8]