Skip to content

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

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 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 1 commit
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
55 changes: 23 additions & 32 deletions src/localmethtable.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,17 +11,17 @@ 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
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::FrameInstance
matches = !is_generated(scopeof(fi.framecode)) || enter_generated == fi.enter_generated
if matches
for i = 1:nargs
if !isa(fargs[i], sig[i])
Expand All @@ -34,18 +34,18 @@ 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
return mi.framecode, mi.sparam_vals
return fi.framecode, fi.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
Expand All @@ -55,33 +55,24 @@ function get_call_framecode(fargs::Vector{Any}, parentframe::FrameCode, idx::Int
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
fi = FrameInstance(framecode, env, is_generated(scopeof(framecode)) && enter_generated)
d_meth = DispatchableMethod(nothing, fi, argtypes)
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] = tme
parentframe.methodtables[idx] = d_meth
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
14 changes: 10 additions & 4 deletions src/types.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,12 @@ function breakpointchar(bps::BreakpointState)
return bps.condition === falsecondition ? ' ' : 'd' # no breakpoint : disabled
end

mutable struct DispatchableMethod
next::Union{Nothing,DispatchableMethod} # linked-list representation
frameinstance::Any # really a FrameInstance but we have a cyclic dependency
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 +74,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 +86,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 @@ -331,7 +337,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 +375,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