Skip to content

help load time regression in #33615 #33757

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
merged 1 commit into from
Nov 5, 2019
Merged

help load time regression in #33615 #33757

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
2 changes: 1 addition & 1 deletion base/Base.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ setproperty!(x::Type, f::Symbol, v) = setfield!(x, f, v)
getproperty(x::Tuple, f::Int) = getfield(x, f)
setproperty!(x::Tuple, f::Int, v) = setfield!(x, f, v) # to get a decent error

getproperty(Core.@nospecialize(x), f::Symbol) = getfield(x, f)
getproperty(x, f::Symbol) = getfield(x, f)
setproperty!(x, f::Symbol, v) = setfield!(x, f, convert(fieldtype(typeof(x), f), v))

include("coreio.jl")
Expand Down
4 changes: 3 additions & 1 deletion base/compiler/tfuncs.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,9 @@ function instanceof_tfunc(@nospecialize(t))
t′ = unwrap_unionall(t)
t′′, isexact = instanceof_tfunc(t′)
tr = rewrap_unionall(t′′, t)
if t′′ isa DataType && !has_free_typevars(tr)
# Note: adding the <:Tuple upper bound in NamedTuple was part of the load time
# regression in #33615.
if t′′ isa DataType && !has_free_typevars(tr) && t′′.name !== NamedTuple_typename
# a real instance must be within the declared bounds of the type,
# so we can intersect with the original wrapper.
tr = typeintersect(tr, t′′.name.wrapper)
Expand Down
2 changes: 1 addition & 1 deletion base/essentials.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -166,7 +166,7 @@ true
function convert end

convert(::Type{Union{}}, x) = throw(MethodError(convert, (Union{}, x)))
convert(::Type{Any}, @nospecialize(x)) = x
convert(::Type{Any}, x) = x
convert(::Type{T}, x::T) where {T} = x
convert(::Type{Type}, x::Type) = x # the ssair optimizer is strongly dependent on this method existing to avoid over-specialization
# in the absence of inlining-enabled
Expand Down
2 changes: 1 addition & 1 deletion base/linked_list.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,7 @@ function list_deletefirst!(q::InvasiveLinkedList{T}, val::T) where T
head_next = head.next
while head_next !== val
head = head_next
head_next = head.next
head_next = head.next::Union{T, Nothing}
end
if q.tail::T === val
head.next = nothing
Expand Down
4 changes: 3 additions & 1 deletion base/promotion.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,9 @@ function typejoin(@nospecialize(a), @nospecialize(b))
if ai === bi || (isa(ai,Type) && isa(bi,Type) && ai <: bi && bi <: ai)
aprimary = aprimary{ai}
else
pushfirst!(vars, aprimary.var)
# pushfirst!(vars, aprimary.var)
_growbeg!(vars, 1)
arrayset(false, vars, aprimary.var, 1)
aprimary = aprimary.body
end
end
Expand Down
2 changes: 1 addition & 1 deletion base/regex.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -643,7 +643,7 @@ function *(r1::Union{Regex,AbstractString,AbstractChar}, rs::Union{Regex,Abstrac
shared = mask
for r in (r1, rs...)
r isa Regex || continue
if match_opts == nothing
if match_opts === nothing
match_opts = r.match_options
compile_opts = r.compile_options & ~mask
else
Expand Down
25 changes: 18 additions & 7 deletions base/strings/io.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,19 +107,30 @@ function sprint(f::Function, args...; context=nothing, sizehint::Integer=0)
String(resize!(s.data, s.size))
end

tostr_sizehint(x) = 8
tostr_sizehint(x::AbstractString) = lastindex(x)
tostr_sizehint(x::Union{String,SubString{String}}) = sizeof(x)
tostr_sizehint(x::Float64) = 20
tostr_sizehint(x::Float32) = 12
# CategoricalArrays extends this
function tostr_sizehint end
Copy link
Member

Choose a reason for hiding this comment

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

So this function isn't used now anymore? It seems somewhat unfortunate to duplicate this code in two places below just to avoid getting that extension. Maybe just

let tostr_sizehint(x) = begin
if x isa Float64
            siz += 20
        elseif x isa Float32
            siz += 12
        elseif x isa String || x isa SubString{String}
            siz += sizeof(x)
        elseif x isa Char
            siz += ncodeunits(x)
        else
            siz += 8
        end
end

around the two function definitions?

Copy link
Member

Choose a reason for hiding this comment

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

Is the point of this change just to ensure CategoricalArrays don't overload this? Just ask if that's what you want -- that's more reliable than keeping an empty definition hoping that I don't notice. :-p

Why is it a problem exactly? I understand that things like Base.convert(::Type{T}, ::CategoricalValue{T}) are hard for the compiler, but Base.tostr_sizehint(::CategoricalString) looks like a much more innocuous definition to me. Note that I don't care so much about tostr_sizehint as I care about convert (it's just a small optimization) so I'm just going to remove it.


function _str_sizehint(x)
if x isa Float64
return 20
elseif x isa Float32
return 12
elseif x isa String || x isa SubString{String}
return sizeof(x)
elseif x isa Char
return ncodeunits(x)
else
return 8
end
end

function print_to_string(xs...)
if isempty(xs)
return ""
end
siz::Int = 0
for x in xs
siz += tostr_sizehint(x)
siz += _str_sizehint(x)
end
# specialized for performance reasons
s = IOBuffer(sizehint=siz)
Expand All @@ -135,7 +146,7 @@ function string_with_env(env, xs...)
end
siz::Int = 0
for x in xs
siz += tostr_sizehint(x)
siz += _str_sizehint(x)
end
# specialized for performance reasons
s = IOBuffer(sizehint=siz)
Expand Down
18 changes: 9 additions & 9 deletions base/task.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,7 +141,7 @@ julia> istaskdone(b)
true
```
"""
istaskdone(t::Task) = ((t.state == :done) | istaskfailed(t))
istaskdone(t::Task) = ((t.state === :done) | istaskfailed(t))

"""
istaskstarted(t::Task) -> Bool
Expand Down Expand Up @@ -182,7 +182,7 @@ julia> istaskfailed(b)
true
```
"""
istaskfailed(t::Task) = (t.state == :failed)
istaskfailed(t::Task) = (t.state === :failed)

Threads.threadid(t::Task) = Int(ccall(:jl_get_task_tid, Int16, (Any,), t)+1)

Expand Down Expand Up @@ -390,7 +390,7 @@ function task_done_hook(t::Task)

if err && !handled && Threads.threadid() == 1
if isa(result, InterruptException) && isdefined(Base, :active_repl_backend) &&
active_repl_backend.backend_task.state == :runnable && isempty(Workqueue) &&
active_repl_backend.backend_task.state === :runnable && isempty(Workqueue) &&
active_repl_backend.in_eval
throwto(active_repl_backend.backend_task, result) # this terminates the task
end
Expand All @@ -405,7 +405,7 @@ function task_done_hook(t::Task)
# issue #19467
if Threads.threadid() == 1 &&
isa(e, InterruptException) && isdefined(Base, :active_repl_backend) &&
active_repl_backend.backend_task.state == :runnable && isempty(Workqueue) &&
active_repl_backend.backend_task.state === :runnable && isempty(Workqueue) &&
active_repl_backend.in_eval
throwto(active_repl_backend.backend_task, e)
else
Expand Down Expand Up @@ -482,7 +482,7 @@ function __preinit_threads__()
end

function enq_work(t::Task)
(t.state == :runnable && t.queue === nothing) || error("schedule: Task not runnable")
(t.state === :runnable && t.queue === nothing) || error("schedule: Task not runnable")
tid = Threads.threadid(t)
# Note there are three reasons a Task might be put into a sticky queue
# even if t.sticky == false:
Expand Down Expand Up @@ -542,13 +542,13 @@ true
"""
function schedule(t::Task, @nospecialize(arg); error=false)
# schedule a task to be (re)started with the given value or exception
t.state == :runnable || Base.error("schedule: Task not runnable")
t.state === :runnable || Base.error("schedule: Task not runnable")
if error
t.queue === nothing || Base.list_deletefirst!(t.queue, t)
t.exception = arg
setfield!(t, :exception, arg)
Copy link
Member

Choose a reason for hiding this comment

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

Why does this help? Does somebody overwrite a setproperty that generic?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's the conversion to Any in setproperty.

else
t.queue === nothing || Base.error("schedule: Task not runnable")
t.result = arg
setfield!(t, :result, arg)
end
enq_work(t)
return t
Expand Down Expand Up @@ -624,7 +624,7 @@ end
function ensure_rescheduled(othertask::Task)
ct = current_task()
W = Workqueues[Threads.threadid()]
if ct !== othertask && othertask.state == :runnable
if ct !== othertask && othertask.state === :runnable
# we failed to yield to othertask
# return it to the head of a queue to be retried later
tid = Threads.threadid(othertask)
Expand Down
4 changes: 2 additions & 2 deletions base/util.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -304,7 +304,7 @@ end

## printing with color ##

const text_colors = AnyDict(
const text_colors = Dict{Union{Symbol,Int},String}(
:black => "\033[30m",
:red => "\033[31m",
:green => "\033[32m",
Expand Down Expand Up @@ -334,7 +334,7 @@ for i in 0:255
text_colors[i] = "\033[38;5;$(i)m"
end

const disable_text_style = AnyDict(
const disable_text_style = Dict{Symbol,String}(
:bold => "\033[22m",
:underline => "\033[24m",
:blink => "\033[25m",
Expand Down
2 changes: 1 addition & 1 deletion stdlib/Distributed/src/Distributed.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import Base: getindex, wait, put!, take!, fetch, isready, push!, length,
hash, ==, kill, close, isopen, showerror

# imports for use
using Base: Process, Semaphore, JLOptions, AnyDict, buffer_writes, @sync_add,
using Base: Process, Semaphore, JLOptions, buffer_writes, @sync_add,
VERSION_STRING, binding_module, atexit, julia_exename,
julia_cmd, AsyncGenerator, acquire, release, invokelatest,
shell_escape_posixly, uv_error, something, notnothing, isbuffered
Expand Down
4 changes: 2 additions & 2 deletions stdlib/Distributed/src/cluster.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -445,7 +445,7 @@ function addprocs(manager::ClusterManager; kwargs...)
end

function addprocs_locked(manager::ClusterManager; kwargs...)
params = merge(default_addprocs_params(), AnyDict(kwargs))
params = merge(default_addprocs_params(), Dict{Symbol,Any}(kwargs))
topology(Symbol(params[:topology]))

if PGRP.topology != :all_to_all
Expand Down Expand Up @@ -510,7 +510,7 @@ function set_valid_processes(plist::Array{Int})
end
end

default_addprocs_params() = AnyDict(
default_addprocs_params() = Dict{Symbol,Any}(
:topology => :all_to_all,
:dir => pwd(),
:exename => joinpath(Sys.BINDIR, julia_exename()),
Expand Down
6 changes: 3 additions & 3 deletions stdlib/Distributed/src/remotecall.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
# This file is a part of Julia. License is MIT: https://julialang.org/license

abstract type AbstractRemoteRef end

"""
client_refs

Expand All @@ -8,9 +10,7 @@ Tracks whether a particular `AbstractRemoteRef`

The `client_refs` lock is also used to synchronize access to `.refs` and associated `clientset` state.
"""
const client_refs = WeakKeyDict{Any, Nothing}() # used as a WeakKeySet

abstract type AbstractRemoteRef end
const client_refs = WeakKeyDict{AbstractRemoteRef, Nothing}() # used as a WeakKeySet

"""
Future(w::Int, rrid::RRID, v::Union{Some, Nothing}=nothing)
Expand Down