Conversation
base/reflection.jl
Outdated
|
|
||
| MethodTable(m::Method) = get_methodtable(m.sig) | ||
|
|
||
| get_methodtable(u::UnionAll) = get_methodtable(u.body) |
There was a problem hiding this comment.
There's an existing function for this that should be reused.
There was a problem hiding this comment.
The suspense is killing me, what is it? I changed it to the closest thing I could find, inspired by code in serialize.jl. There's also this sequence in methodshow. I grepped base (grep -R "\.mt\b" *) and src (grep -R DLLEXPORT jl_methtable_t *) without noticing any better hits.
3357f95 to
b225a5e
Compare
|
Thanks, Jeff, for looking at this. I noticed one more thing, there's a trivial
I'll go with the |
The method is still correct. You cannot delete a Method object, you can only simply disable it in the lookup tables. |
|
What @vtjnash says seems correct to me. If you have a method object calling it should continue to do what it always did. Deleting a method isn't about disabling that method altogether, it's about removing it from a generic function's method table. |
|
Sounds good to me, and definitely makes my life easier. Barring any further objections I'll merge tomorrow. |
|
There's still the issue of loading |
|
Just to make sure I understand, is this an adequate test case? A.jl:__precompile__()
module A
export afunc
afunc(::Int, ::Int) = 1
afunc(::Any, ::Any) = 2
endB.jl__precompile__()
module B
using A
export bfunc
bfunc(x) = afunc(x, x)
precompile(bfunc, (Int,))
precompile(bfunc, (Float64,))
endAnd then: julia> using A
julia> mths = collect(methods(afunc))
2-element Array{Any,1}:
afunc(::Int64, ::Int64) in A at /tmp/A.jl:7
afunc(::Any, ::Any) in A at /tmp/A.jl:8
julia> Base.delete_method(mths[1])
julia> using B
# Currently hangs |
ebeda18 to
dce0648
Compare
|
OK, I think I fixed it (see the second commit). Once I figured out what I was doing, it seemed surprisingly easy. If I'm not completely fooling myself, the credit is due to the designer(s) for setting up an architecture that makes it easy to fail and then recover at the right place (and having that recovery already available). |
src/dump.c
Outdated
| _new = (jl_method_t*)jl_methtable_lookup(mt, sig, world); | ||
| assert(_new && jl_is_method(_new)); | ||
| if (!(_new && jl_is_method(_new))) | ||
| return NULL; // the method wasn't found, probably because it was disabled |
There was a problem hiding this comment.
This function absolutely must return a valid object, otherwise we're leaving a field somewhere with uninitialized memory (also, incidentally, with the wrong typeof).
There was a problem hiding this comment.
I do check for a NULL return in all callers of this method, is that not good enough? The overall strategy I took is to fail in a way that signals to jl_insert_backedges and have it drop any dependent methods.
If it really must return a valid method, then I could return to an intermediate version of this PR (before I discovered what I thought was a clever strategy). There I added a max_world_mask to jl_typemap_assoc/lookup_by_type and then re-ran the lookup to ignore max_world in deciding whether a method fits. Not quite sure what it will grab, but it will grab something.
|
OK, perhaps this 3rd commit does the trick? The one thing I am concerned about is what happens when the match is a |
600e589 to
d16fb9a
Compare
|
I tried loading several packages with a breakpoint set in |
|
If further objections do not arise, I'll merge soon. |
This is thanks to a fortuitously-timed trip to Boston and @vtjnash kindly spending a Sunday afternoon guiding me through this. I used it heavily today (with a local branch of Revise), and after I fixed one or two bugs I seemed to be running out of ways to break Julia 😄.
Technically this doesn't delete the method, it simply sets
max_worldto be too small to run in the future. The most subtle part of this is that you have to recompute ambiguities, because if you delete an ambiguity-resolving method you might suddenly need to start throwingMethodErrors as appropriate. A naive computation of the new ambiguities isO(N^2)in the number of methods; we found we could rebuild the entire method table forconvert(>700 methods) in ~30ms, so even the naive approach would probably have been workable. Nevertheless, we decided to limit it to those that intersect with the signature that's being deleted, so it'sO(n^2)wherenis the number that intersect. For most cases this should be pretty cheap.