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

use our own struct for the local method table instead of TypeMapEntry #313

Merged
merged 3 commits into from
Aug 16, 2019
Merged

use our own struct for the local method table instead of TypeMapEntry #313

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
96e0194
use our own struct for the local method table instead of TypeMapEntry
KristofferC Jul 31, 2019
4a704d2
also cache calls that end up getting compiled in local method table
KristofferC Jul 31, 2019
e6e64f2
add a typeassert
KristofferC Jul 31, 2019
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 src/JuliaInterpreter.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ module JuliaInterpreter

using Base.Meta
import Base: +, -, convert, isless
using Core: CodeInfo, TypeMapEntry, SimpleVector, LineInfoNode, GotoNode, Slot,
using Core: CodeInfo, SimpleVector, LineInfoNode, GotoNode, Slot,
GeneratedFunctionStub, MethodInstance, NewvarNode, TypeName

using UUIDs
Expand Down
6 changes: 3 additions & 3 deletions src/construct.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -248,7 +248,7 @@ function prepare_call(@nospecialize(f), allargs; enter_generated = false)
# The generator threw an error. Let's generate the same error by calling it.
f(allargs[2:end]...)
end
isa(ret, Compiled) && return ret
isa(ret, Compiled) && return ret, argtypes
# Typical return
framecode, lenv = ret
if is_generated(method) && enter_generated
Expand Down Expand Up @@ -543,7 +543,7 @@ See [`enter_call`](@ref) for a similar approach not based on expressions.
function enter_call_expr(expr; enter_generated = false)
clear_caches()
r = determine_method_for_expr(expr; enter_generated = enter_generated)
if isa(r, Tuple)
if r !== nothing && !isa(r[1], Compiled)
return prepare_frame(r[1:end-1]...)
end
nothing
Expand Down Expand Up @@ -597,7 +597,7 @@ function enter_call(@nospecialize(finfo), @nospecialize(args...); kwargs...)
error(f, " is a builtin or intrinsic")
end
r = prepare_call(f, allargs; enter_generated=enter_generated)
if isa(r, Tuple)
if r !== nothing && !isa(r[1], Compiled)
return prepare_frame(r[1:end-1]...)
end
return nothing
Expand Down
81 changes: 45 additions & 36 deletions src/localmethtable.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,17 +11,22 @@ function get_call_framecode(fargs::Vector{Any}, parentframe::FrameCode, idx::Int
nargs = length(fargs) # includes f as the first "argument"
# Determine whether we can look up the appropriate framecode in the local method table
if isassigned(parentframe.methodtables, idx) # if this is the first call, this may not yet be set
tme = tme1 = parentframe.methodtables[idx]::TypeMapEntry
local tmeprev
# The case where `methodtables[idx]` is a `Compiled` has already been handled in `bypass_builtins`
d_meth = d_meth1 = parentframe.methodtables[idx]::DispatchableMethod
local d_methprev
depth = 1
while true
# TODO: consider using world age bounds to handle cache invalidation
# Determine whether the argument types match the signature
sig = tme.sig.parameters::SimpleVector
sig = d_meth.sig.parameters::SimpleVector
if length(sig) == nargs
# If this is generated, match only if `enter_generated` also matches
mi = tme.func::FrameInstance
matches = !is_generated(scopeof(mi.framecode)) || enter_generated == mi.enter_generated
fi = d_meth.frameinstance
if fi isa FrameInstance
matches = !is_generated(scopeof(fi.framecode)) || enter_generated == fi.enter_generated
else
matches = !enter_generated
end
if matches
for i = 1:nargs
if !isa(fargs[i], sig[i])
Expand All @@ -34,54 +39,58 @@ function get_call_framecode(fargs::Vector{Any}, parentframe::FrameCode, idx::Int
# Rearrange the list to place this method first
# (if we're in a loop, we'll likely match this one again on the next iteration)
if depth > 1
parentframe.methodtables[idx] = tme
tmeprev.next = tme.next
tme.next = tme1
parentframe.methodtables[idx] = d_meth
d_methprev.next = d_meth.next
d_meth.next = d_meth1
end
if fi isa Compiled
return Compiled(), nothing
else
return fi.framecode, fi.sparam_vals
end
return mi.framecode, mi.sparam_vals
end
end
depth += 1
tmeprev = tme
tme = tme.next
tme === nothing && break
tme = tme::TypeMapEntry
d_methprev = d_meth
d_meth = d_meth.next
d_meth === nothing && break
d_meth = d_meth::DispatchableMethod
end
end
# We haven't yet encountered this argtype combination and need to look it up by dispatch
fargs[1] = f = to_function(fargs[1])
ret = prepare_call(f, fargs; enter_generated=enter_generated)
ret === nothing && return f(fargs[2:end]...), nothing
isa(ret, Compiled) && return ret, nothing
framecode, args, env, argtypes = ret
# Store the results of the method lookup in the local method table
mi = FrameInstance(framecode, env, is_generated(scopeof(framecode)) & enter_generated)
# it's sort of odd to call this a TypeMapEntry, then set most of the fields incorrectly
# but since we're just using it as a linked list, it's probably ok
tme = ccall(:jl_new_struct_uninit, Any, (Any,), TypeMapEntry)::TypeMapEntry
tme.func = mi
tme.simplesig = nothing
tme.sig = argtypes
tme.isleafsig = true
tme.issimplesig = false
method = framecode.scope::Method
tme.va = method.isva
is_compiled = isa(ret[1], Compiled)
local framecode
if is_compiled
d_meth = DispatchableMethod(nothing, Compiled(), ret[2])
else
framecode, args, env, argtypes = ret
# Store the results of the method lookup in the local method table
fi = FrameInstance(framecode, env, is_generated(scopeof(framecode)) && enter_generated)
d_meth = DispatchableMethod(nothing, fi, argtypes)
end
if isassigned(parentframe.methodtables, idx)
tme.next = parentframe.methodtables[idx]
# Drop the oldest tme, if necessary
tmetmp = tme.next
d_meth.next = parentframe.methodtables[idx]
# Drop the oldest d_meth, if necessary
d_methtmp = d_meth.next
depth = 2
while isdefined(tmetmp, :next) && tmetmp.next !== nothing
while d_methtmp.next !== nothing
depth += 1
tmetmp = tmetmp.next
d_methtmp = d_methtmp.next
depth >= max_methods && break
end
if depth >= max_methods
tmetmp.next = nothing
d_methtmp.next = nothing
end
else
tme.next = nothing
d_meth.next = nothing
end
parentframe.methodtables[idx] = d_meth
if is_compiled
return Compiled(), nothing
else
return framecode, env
end
parentframe.methodtables[idx] = tme
return framecode, env
end
2 changes: 1 addition & 1 deletion src/optimize.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -232,7 +232,7 @@ function optimize!(code::CodeInfo, scope)
code.ssavaluetypes = length(new_code)

# Insert the foreigncall wrappers at the updated idxs
methodtables = Vector{Union{Compiled,TypeMapEntry}}(undef, length(code.code))
methodtables = Vector{Union{Compiled,DispatchableMethod}}(undef, length(code.code))
for idx in foreigncalls_idx
methodtables[ssalookup[idx]] = Compiled()
end
Expand Down
17 changes: 12 additions & 5 deletions src/types.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,13 @@ function breakpointchar(bps::BreakpointState)
return bps.condition === falsecondition ? ' ' : 'd' # no breakpoint : disabled
end

abstract type AbstractFrameInstance end
mutable struct DispatchableMethod
next::Union{Nothing,DispatchableMethod} # linked-list representation
frameinstance::Union{Compiled, AbstractFrameInstance} # really a Union{Compiled, FrameInstance} but we have a cyclic dependency
Copy link
Member

Choose a reason for hiding this comment

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

This could hurt. Union-splitting may not work, unless the compiler is smart enough to know there is only one concrete subtype.

Copy link
Member Author

@KristofferC KristofferC Jul 31, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah but we type assert immediately when we use it anyway but I will time some stuff and see if I can do it some other way.

Where the stuff in TypeMapEntry inferrable?

Copy link
Member Author

Choose a reason for hiding this comment

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

Looked a bit, there is a single place where we look at this field and all uses either just calls isa on it, or type asserts before it is used so there should be no dynamic behavior.

sig::Type # for speed of matching, this is a *concrete* signature. `sig <: frameinstance.framecode.scope.sig`
Copy link
Member

Choose a reason for hiding this comment

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

If you're doing this, can you implement the compiled idea too? With this diff

tim@diva:~/.julia/dev/JuliaInterpreter$ git diff
diff --git a/src/construct.jl b/src/construct.jl
index 9b70786..796c4ab 100644
--- a/src/construct.jl
+++ b/src/construct.jl
@@ -236,6 +236,7 @@ function prepare_call(@nospecialize(f), allargs; enter_generated = false)
     end
     argtypesv = Any[_Typeof(a) for a in allargs]
     argtypes = Tuple{argtypesv...}
+    println("Calling which (super-slow)")
     method = whichtt(argtypes)
     if method === nothing
         # Call it to generate the exact error

and the following test code

using JuliaInterpreter

bar(::Int) = 1
bar(::Float64) = 2
push!(JuliaInterpreter.compiled_methods, @which bar(1.0))

function foo(A)
    s = 0
    for a in A
        @bp
        s += bar(a)
    end
    return s
end
A = Real[1, 1, 1, 1, 1.0, 1.0, 1.0, 1.0]
frame = JuliaInterpreter.enter_call(foo, A)
for i = 1:length(A)
    println("iteration ", i)
    debug_command(frame, :c)
    for i = 1:3
        debug_command(frame, :se)  # step forward to the call to bar
    end
    # display(frame.framecode.methodtables)
end

I think you'll see what I'm worried about. (Note that the Int method of bar is recursively interpreted and the Float64 one in Compiled mode.) If you inspect frame.framecode.methodtables you'll see that Compiled methods can't exploit the local method tables.

In the longer run we want to store the MethodInstance, and then use a ccall to skip the jl_lookup_generic from jl_apply_generic. That would basically mean DLLEXPORTing this. That will eliminate the runtime dispatch on compiled dispatch from the interpreter.

Copy link
Member

Choose a reason for hiding this comment

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

Correction: there's already this, so we don't have to modify Julia at all.

Copy link
Member Author

@KristofferC KristofferC Jul 31, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I added caching for Compiled calls as well.

Copy link
Member Author

@KristofferC KristofferC Jul 31, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

function jl_generic(args)
    args[1] + args[2]
end

function jl_invoke(args, mi)
     ccall(:jl_invoke, Any, (Any, Ptr{Any}, Cuint, Any), +, args, 2, mi)
end

args = Any[1, 2]
@btime jl_generic($args)
# 25 ns
mi = Core.Compiler.specialize_method(@which(1 + 1), Tuple{typeof(+), Int, Int}, Core.svec())
@btime  jl_invoke($args, $mi)
# 11 ns

Copy link
Member

Choose a reason for hiding this comment

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

Not quite sure I get what foo is, but if you're testing jl_invoke against run-time dispatch keep in mind that the timings are rather dependent on how many methods there are. convert might be in a different category 😄.

Copy link
Member Author

@KristofferC KristofferC Jul 31, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Sorry, had a typo, foo should have been jl_invoke. Just wanted to show that using jl_invoke with a MethodInstance was indeed faster :)

end

"""
`FrameCode` holds static information about a method or toplevel code.
One `FrameCode` can be shared by many calling `Frame`s.
Expand All @@ -68,7 +75,7 @@ Important fields:
struct FrameCode
scope::Union{Method,Module}
src::CodeInfo
methodtables::Vector{Union{Compiled,TypeMapEntry}} # line-by-line method tables for generic-function :call Exprs
methodtables::Vector{Union{Compiled,DispatchableMethod}} # line-by-line method tables for generic-function :call Exprs
Copy link
Member

Choose a reason for hiding this comment

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

I don't see how this prevents which-based lookup on each iteration of a loop. In #309 I added a compiled field to DispatchableMethod, which seems like the right place for this information (once you've determined that a method matches, your next question is whether you should run it in interpreted or compiled mode).

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 prevents it because frameinstance in DispatchableMethodis a Compiled in those cases.

Copy link
Member Author

Choose a reason for hiding this comment

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

The reason I still want to have Compiled directly in methodtables is for ccall / fcall where we know for sure that the method we want to call are compiled so there is no need to do any matching against signatures.

breakpoints::Vector{BreakpointState}
used::BitSet
generator::Bool # true if this is for the expression-generator of a @generated function
Expand All @@ -80,7 +87,7 @@ function FrameCode(scope, src::CodeInfo; generator=false, optimize=true)
src, methodtables = optimize!(copy_codeinfo(src), scope)
else
src = replace_coretypes!(copy_codeinfo(src))
methodtables = Vector{Union{Compiled,TypeMapEntry}}(undef, length(src.code))
methodtables = Vector{Union{Compiled,DispatchableMethod}}(undef, length(src.code))
end
breakpoints = Vector{BreakpointState}(undef, length(src.code))
for (i, pc_expr) in enumerate(src.code)
Expand Down Expand Up @@ -118,7 +125,7 @@ Fields:
- `framecode`: the [`FrameCode`](@ref) for the method.
- `sparam_vals`: the static parameter values for the method.
"""
struct FrameInstance
struct FrameInstance <: AbstractFrameInstance
framecode::FrameCode
sparam_vals::SimpleVector
enter_generated::Bool
Expand Down Expand Up @@ -331,7 +338,7 @@ struct BreakpointSignature <: AbstractBreakpoint
enabled::Ref{Bool}
instances::Vector{BreakpointRef}
end
same_location(bp2::BreakpointSignature, bp::BreakpointSignature) =
same_location(bp2::BreakpointSignature, bp::BreakpointSignature) =
bp2.f == bp.f && bp2.sig == bp.sig && bp2.line == bp.line
function Base.show(io::IO, bp::BreakpointSignature)
print(io, bp.f)
Expand Down Expand Up @@ -369,7 +376,7 @@ struct BreakpointFileLocation <: AbstractBreakpoint
enabled::Ref{Bool}
instances::Vector{BreakpointRef}
end
same_location(bp2::BreakpointFileLocation, bp::BreakpointFileLocation) =
same_location(bp2::BreakpointFileLocation, bp::BreakpointFileLocation) =
bp2.path == bp.path && bp2.abspath == bp.abspath && bp2.line == bp.line
function Base.show(io::IO, bp::BreakpointFileLocation)
print(io, bp.path, ':', bp.line)
Expand Down