help load time regression in #33615 #33757
Conversation
| if error | ||
| t.queue === nothing || Base.list_deletefirst!(t.queue, t) | ||
| t.exception = arg | ||
| setfield!(t, :exception, arg) |
There was a problem hiding this comment.
Why does this help? Does somebody overwrite a setproperty that generic?
There was a problem hiding this comment.
It's the conversion to Any in setproperty.
| tostr_sizehint(x::Float64) = 20 | ||
| tostr_sizehint(x::Float32) = 12 | ||
| # CategoricalArrays extends this | ||
| function tostr_sizehint end |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
base/linked_list.jl
Outdated
| @@ -97,10 +97,10 @@ function list_deletefirst!(q::InvasiveLinkedList{T}, val::T) where T | |||
| q.head = val.next::T | |||
| end | |||
| else | |||
| head_next = head.next | |||
| head_next = head.next::Union{T, Nothing} | |||
There was a problem hiding this comment.
Why does inference have trouble figuring this one out?
There was a problem hiding this comment.
Hmm, that's a good question. I thought the type of the field was Any, but it's not. Nevertheless we get:
7 ── %23 = Base.getfield(%7, :next)::Union{Task, Nothing}
8 ┄─ %24 = φ (#7 => %7, #9 => %25)::Any
│ %25 = φ (#7 => %23, #9 => %29)::Any
│ %26 = (%25 === val)::Bool
│ %27 = Core.Intrinsics.not_int(%26)::Bool
└─── goto #10 if not %27
9 ── %29 = Base.getproperty(%25, :next)::Any
└─── goto #8
|
The changes lgtm, but I agree it’d be good to have some commentary too on why these changes. |
JeffBezanson
force-pushed
the
jb/33615
branch
from
fc76fa1 to
ad8ba12
Compare
Deprecate it in favor of `CategoricalValue{String}`. Keep all deprecations
and add new ones so that most functionality keeps working. The main breaking
change which cannot be avoided is the inheritance from `AbstractString`.
Stop overloading `Base.tostr_sizehint` as it appears to trigger lots of recompilation
and it's just a small optimization (see JuliaLang/julia#33757).
Deprecate it in favor of `CategoricalValue{String}`. Keep all deprecations
and add new ones so that most functionality keeps working. The main breaking
change which cannot be avoided is the inheritance from `AbstractString`.
Stop overloading `Base.tostr_sizehint` as it appears to trigger lots of recompilation
and it's just a small optimization (see JuliaLang/julia#33757).
For me this reduces the load time of CSV after Atom from ~11 seconds to ~6 seconds.