use our own struct for the local method table instead of TypeMapEntry #313
Conversation
Codecov Report
@@ Coverage Diff @@
## master #313 +/- ##
==========================================
+ Coverage 88.17% 88.18% +0.01%
==========================================
Files 11 11
Lines 1742 1744 +2
==========================================
+ Hits 1536 1538 +2
Misses 206 206
Continue to review full report at Codecov.
|
| 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` |
There was a problem hiding this comment.
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 errorand 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)
endI 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.
There was a problem hiding this comment.
Correction: there's already this, so we don't have to modify Julia at all.
There was a problem hiding this comment.
I added caching for Compiled calls as well.
There was a problem hiding this comment.
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 nsThere was a problem hiding this comment.
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 😄.
There was a problem hiding this comment.
Sorry, had a typo, foo should have been jl_invoke. Just wanted to show that using jl_invoke with a MethodInstance was indeed faster :)
| 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` |
There was a problem hiding this comment.
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 😄.
| @@ -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 | |||
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
It prevents it because frameinstance in DispatchableMethodis a Compiled in those cases.
There was a problem hiding this comment.
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.
| 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 |
There was a problem hiding this comment.
This could hurt. Union-splitting may not work, unless the compiler is smart enough to know there is only one concrete subtype.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
|
Thanks for doing this and all the other PRs. I've been out on vacation (still am, actually), it's great to have the package in such good hands! |
No performance difference but I feel this should be easier to maintain.
Slightly lifted from #309