Support method deletion

[timholy]

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_world to 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 throwing MethodErrors as appropriate. A naive computation of the new ambiguities is O(N^2) in the number of methods; we found we could rebuild the entire method table for convert (>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's O(n^2) where n is the number that intersect. For most cases this should be pretty cheap.

[JeffBezanson]

There's an existing function for this that should be reused.

[timholy]

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.

[timholy]

Thanks, Jeff, for looking at this.

I noticed one more thing, there's a trivial max_world(m::Method) which is no longer correct if you deleted m but are still holding on to it as a variable. (methods will not return invalid methods, so you have to do a little bit of work to get them.) While min_world is stored in Method, max_world is not. Possible actions:

• add a max_world entry to Method
• get the corresponding TypeMapEntry and return it from there
• delete max_world(::Method)

I'll go with the TypeMapEntry approach if I don't hear otherwise.

[vtjnash]

which is no longer correct if you deleted m

The method is still correct. You cannot delete a Method object, you can only simply disable it in the lookup tables.

[StefanKarpinski]

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.

[timholy]

Sounds good to me, and definitely makes my life easier.

Barring any further objections I'll merge tomorrow.

[vtjnash]

There's still the issue of loading .ji files to be dealt with. Somehow it needs to compute the result of method lookup as-if the deletion never happened.

[timholy]

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

end

B.jl

__precompile__()

module B

using A
export bfunc

bfunc(x) = afunc(x, x)

precompile(bfunc, (Int,))
precompile(bfunc, (Float64,))

end

And 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

[timholy]

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).

[vtjnash]

This function absolutely must return a valid object, otherwise we're leaving a field somewhere with uninitialized memory (also, incidentally, with the wrong typeof).

[timholy]

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.

[timholy]

OK, perhaps this 3rd commit does the trick? The one thing I am concerned about is what happens when the match is a Method rather than a MethodInstance; in that case I don't seem to have a way of signaling back that there was a world age problem.

[timholy]

I tried loading several packages with a breakpoint set in jl_recache_method and never hit it. Not quite sure what needs to be done, if anything, to test this.

[timholy]

If further objections do not arise, I'll merge soon.