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.

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

A grabbag of random fixes #212

Merged
merged 5 commits into from
Jan 12, 2021
Merged

A grabbag of random fixes #212

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
d7ec074
Fix some demo/test mistakes
timholy Jan 8, 2021
cf1a0ff
better exclusion of toplevel
timholy Jan 11, 2021
5dad1a5
Check empty stackframes in skiphigherorder
timholy Jan 11, 2021
a3c6d32
Check for empty flamegraph
timholy Jan 11, 2021
fb4c0aa
Some known_type fixes
timholy Jan 11, 2021
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 examples/OptimizeMe.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ module OptimizeMe
struct Container{T}
value::T
end

concat_string(c1::Container, c2::Container) = string(c1.value) * ' ' * string(c2.value)

function contain_concrete(item1, item2)
Expand All @@ -31,7 +32,6 @@ end
struct Object
x::Int
end
Base.show(io::IO, o::Object) = print(io, "Object x: ", o.x)

function makeobjects()
xs = [1:5; 7]
Expand Down
3 changes: 2 additions & 1 deletion src/parcel_snoopi.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -214,10 +214,11 @@ let known_type_cache = IdDict{Tuple{Module,Tuple{Vararg{Symbol}},Symbol},Bool}()
strippedname(tn::Core.TypeName) = Symbol(string(tn.name)[2:end])

T === Union{} && return true
T = Base.unwrap_unionall(T)
if isa(T, Union)
return known_type(mod, T.a) & known_type(mod, T.b)
end
T = Base.unwrap_unionall(T)::DataType
T = T::DataType
tn = T.name
tpath = fullname(tn.module)
key = (mod, tpath, tn.name)
Expand Down
16 changes: 11 additions & 5 deletions src/parcel_snoopi_deep.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -668,7 +668,11 @@ function maybe_internal(itrig::InferenceTrigger)
if isa(linfo, MethodInstance)
m = linfo.def
if isa(m, Method)
m.module === Base && m.name === :include_string && return false
if m.module === Base
m.name === :include_string && return false
m.name === :_include_from_serialized && return false
m.name === :return_types && return false # from `@inferred`
end
m.name === :eval && return false
end
end
Expand Down Expand Up @@ -747,9 +751,11 @@ function skiphigherorder(itrig::InferenceTrigger; exact::Bool=true)
ft = Base.unwrap_unionall(Base.unwrap_unionall(MethodInstance(itrig.node).specTypes).parameters[1])
sfs, idx = itrig.callerframes, itrig.btidx
while idx < length(itrig.node.bt)
callermi = sfs[end].linfo
if !hasparameter(callermi.specTypes, ft, exact)
return InferenceTrigger(itrig.node, sfs, idx)
if !isempty(sfs)
callermi = sfs[end].linfo
if !hasparameter(callermi.specTypes, ft, exact)
return InferenceTrigger(itrig.node, sfs, idx)
end
end
ret = next_julia_frame(itrig.node.bt, idx)
ret === nothing && return InferenceTrigger(itrig.node, sfs, idx)
Expand Down Expand Up @@ -905,8 +911,8 @@ The empty horizontal periods in the flamegraph correspond to times when somethin
The total width of the flamegraph is set from the `ROOT` node.
"""
function FlameGraphs.flamegraph(tinf::InferenceTimingNode; tmin = 0.0, excluded_modules=Set([Main::Module]), mode=nothing)
isROOT(tinf) && isempty(tinf.children) && error("root node has no children")
Copy link
Collaborator

Choose a reason for hiding this comment

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

@timholy I just hit this error in usage of this package..

It seems to me that it's much better to just return an otherwise empty profile, with just the single ROOT node.
From what i can tell, without this check, it does return that empty profile correctly.

Can we remove this line? I'm curious if you remember the motivation to add it.

Copy link
Owner Author

Choose a reason for hiding this comment

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

I don't. I guess I was thinking the error explained what was going on, whereas a blank ProfileView window might just be confusing. For me the usual invocation is ProfileView.view(flamegraph(tinf)).

Copy link
Owner Author

Choose a reason for hiding this comment

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

Very happy to have you change this though!

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, makes sense. I think i was thinking that if it's empty, then it's empty! 😅 But you're right that it's not as nice as if it said "hey this is empty."

Maybe we can print a warning instead of throwing an error?

Our use case is that we're creating this via an HTTP endpoint on prod servers now (!!), and i don't want that to throw an error and return a 500:
JuliaPerf/ProfileEndpoints.jl#16

K, thanks, i'll send a quick PR

io = IOBuffer()

# Compute a "root" frame for the top-level node, to cover the whole profile
node_data, _ = _flamegraph_frame(io, tinf, tinf.start_time, true, excluded_modules, mode; toplevel=true)
root = Node(node_data)
Expand Down
8 changes: 4 additions & 4 deletions test/snoopi_deep.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -329,7 +329,7 @@ end
tinf_spec = @snoopi_deep SnoopBench.mappushes(SnoopBench.spell_spec, Ts)
tf_unspec = flatten(tinf_unspec)
tf_spec = flatten(tinf_spec)
@test length(tf_unspec) < 10
@test length(tf_unspec) < length(Ts) ÷ 5
@test any(tmi -> occursin("spell_unspec(::Any)", repr(MethodInstance(tmi))), tf_unspec)
@test length(tf_spec) >= length(Ts)
@test !any(tmi -> occursin("spell_spec(::Any)", repr(MethodInstance(tmi))), tf_unspec)
Expand Down Expand Up @@ -364,8 +364,8 @@ end
@test tiunspec < tispec/10
@test trunspec < 10*trspec
@test nunspec == 1
# Test that no runtime dispatch occurs in mappushes!
trmp, trtdmp, _, _ = rit[findfirst(pr -> pr.first == mp, rit)].second
@test trtdmp == 0
# Test that little runtime dispatch occurs in mappushes!
_, trtdmpu, _, _ = rit[findfirst(pr -> pr.first == mp, rit)].second
@test trtdmpu < trtdmp/10
# specialization_plot(axs[2], rit; bystr="Inclusive", consts=true, interactive=false)
end
4 changes: 2 additions & 2 deletions test/testmodules/SnoopBench.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,15 +26,15 @@ mappushes3(@nospecialize(f), src) = mappushes3!(f, [], src)
function spell_spec(::Type{T}) where T
name = Base.unwrap_unionall(T).name.name
str = ""
for c in str
for c in string(name)
str *= c
end
return str
end
function spell_unspec(@nospecialize(T))
name = Base.unwrap_unionall(T).name.name
str = ""
for c in str
for c in string(name)
str *= c
end
return str
Expand Down