Support type renaming

[timholy]

Several of us have found Revise.jl to be a significant productivity booster. In principle, it's now possible to keep a Julia session open for a week or more, at which point one of the main negatives of Julia---the cost of JITting your, e.g., plotting package---becomes a non-issue.

However, there are two events which prevent this from being commonplace. One is method-deletion (#20048) and the other is type redefinition (timholy/Revise.jl#18). It occurred to me that type redefinition may not be quite as nasty a problem as I've thought. A demo with this PR:

julia> struct Mine
           arg
       end

julia> a = Mine(1)
Mine(1)

julia> foo(val::Mine) = val.arg
foo (generic function with 1 method)

julia> Base.shunt_binding(Main, :Mine)
Mine#1

julia> a
Mine#1(1)

julia> foo(a)
1

julia> struct Mine{T}
           arg::T
       end

julia> x = Mine(3)
Mine{Int64}(3)

The only catch I'm aware of is:

julia> foo(x)
ERROR: MethodError: no method matching foo(::Mine{Int64})
Closest candidates are:
  foo(::Mine#1) at REPL[3]:1

However, I wonder if this might be solvable via methodswith plus maintaining a cache of the source-code expressions so that they can be re-evaluated. (Revise would like that anyway, since it re-parses and caches every source file so that it can detect diffs. This adds considerably to the package load time, but caching the Exprs to the .ji file seems both cheap and effective.)

[Keno]

However, I wonder if this might be solvable via methodswith plus maintaining a cache of the source-code expressions so that they can be re-evaluated. (Revise would like that anyway, since it re-parses and caches every source file so that it can detect diffs. This adds considerably to the package load time, but caching the Exprs to the .ji file seems both cheap and effective.)

FWIW, for the debugger, I'd like to have the original text source as well.

[cstjean]

FWIW, for the debugger, I'd like to have the original text source as well.

Same here, for a tracing package. Parsing arbitrary source files is no fun.

[vtjnash]

Wouldn't it be easier just to unconditionally emit a warning for redefining a type name / constant?

[timholy]

Revise does, and the user then restarts the session. That's not the end of the world, but if you've got ~1min worth of JITting to get back to where you were, it's not ideal. That's sort of the point of this issue.

Whether it's worth the effort it would take to change this is a separate question.

[timholy]

I stripped the RFC from this, because I'm actually pretty close to getting this working within Revise (see timholy/Revise.jl#25), so now I genuinely want to merge this.

The only part that's missing is a way of going from a Method to the source, which in the absence of a meth.original_source field (which IMO would be the far better way to do it) I propose to do by building up a method => (mod, expr) Dict by parsing the source and then extracting the corresponding method from the method table with methods. Consequently, I'd like to turn

ex = quote
function foo(a::AbstractVector{T}, b::T) where T
    blah blah
end
end

into a method type signature. Funny I've never noticed this before, but do we have an eval_signature_type defined somewhere? A brief glance suggested this isn't pre-packaged in an accessible format.

[JeffBezanson]

Would need a call to unwrap_unionall here.

[JeffBezanson]

If this is used to redefine a type to have a different number of parameters, then re-evaluating existing method definitions won't generally work.

[vtjnash]

I don't think we want to permit deleting an existing binding. We optimize pretty heavily based on the premise that a binding can't be renamed, so this seems still very likely to run into segfaults. At the very least, this method would need to unconditionally print a warning that Julia is now likely to crash.

In #8870 (comment), Stefan suggested we permit replacing bindings for constants (with a warning). This allows code that was compiled for the old binding to continue working (kind of), while making it possible to redefine constants (like Type declarations) and probably not usually manage for the environment to keep working.

[timholy]

If this is used to redefine a type to have a different number of parameters, then re-evaluating existing method definitions won't generally work.

Definitely. Will have to throw an informative error, but the nice thing is that in an interactively-revised session you can get there incrementally.

I don't think we want to permit deleting an existing binding

This doesn't delete anything, it just gives the old type a "deprecated" name (making comment characters part of the type name) so that the original name can be (re)used for the new type. It's a better choice than module reloading, see timholy/Revise.jl#18 (comment), where the Old and New modules print identically and thus yield confusing behavior for users when they wonder why old variables (defined using types exported from the Old module) don't trigger the modified methods in the New module. Here, any objects created in terms of Old types will print with their deprecated name, e.g., typeof(a) = Mine#RV#1, so it should be much clearer what's going on.

We optimize pretty heavily based on the premise that a binding can't be renamed, so this seems still very likely to run into segfaults.

Can you link me to an example? In the admittedly few cases I've looked at, all the ASTs quite gracefully transition over to the "deprecated" name. In the example above, if I define & call bar, its specializations just migrate to the "deprecated" name:

julia> bar(x) = x.arg

julia> bar(a)
1

julia> m = first(methods(bar))
bar(x) in Main at REPL[4]:1

julia> m.specializations
TypeMapEntry(nothing, Tuple{#bar,Mine}, nothing, svec(), 22151, -1, MethodInstance for bar(::Mine), true, true, false)

julia> Base.rename_binding(Main, :Mine, Symbol("Mine#RV#1"))
Mine#RV#1

julia> m.specializations
TypeMapEntry(nothing, Tuple{#bar,Mine#RV#1}, nothing, svec(), 22151, -1, MethodInstance for bar(::Mine#1), true, true, false)

even though I didn't declare an explicit type for bar's inputs.

I'm sure I'm thinking about this naively, but my mental picture is that Julia defines the type essentially via a pointer to a struct; I'm not changing that pointer. The struct has a field with a string in it that's the name that gets printed for the user and to compare to keyboarded code. All I'm doing here is changing the string.

[vtjnash]

This doesn't delete anything

It's a deletion plus an addition from the module names table – our binding table doesn't support versioned renaming. It would be OK to do new naming, but that's just doing const NewName = OldName.

where the Old and New modules print identically

How did the type manage to change names in printing? I thought we print the symbol typeof(x).name.name, but you don't seem to be setting that. Changing the module display-name would actually be defensible. We should already do this in the printing code just by checking whether the lookup returns the right object. I like your idea of doing that.

defines the type essentially via a pointer to a struct

Yes, often, but it can do that only because it know that the binding can't change. If you instead did:

bar(x) = Mine(x).arg

Inference needs to know that that Mine will definitely mean exactly the original type Mine, or all bets are off.

[timholy]

It's a deletion plus an addition from the module names table

OK, admittedly I didn't dig into how this "Dict" is used, but conceptually it's a key-replacement, since the value (the binding) doesn't change. Do keys sometimes escape from the hash table? I guess what you're worried about is T -> sym = T.name.name -> store sym somewhere -> at a later point try to look up module.bindings[stored value of sym]? The thing is, in my intended uses of this, I actually want external objects to switch over to the new meaning of sym, so even this doesn't seem like a big problem to me. (It would be a problem for internal types that are used in the middle of the block of code that does all the invalidation etc, see below. But we can simply say that redefining Base/Core types is not supported.)

I thought we print the symbol typeof(x).name.name, but you don't seem to be setting that.

I am, see the diff for base/reflection.jl.

bar(x) = Mine(x).arg ... Inference needs to know that that Mine will definitely mean exactly the original type Mine, or all bets are off.

I think I know what you mean, but I'm not sure. Does this logic in Revise address your concern? (not merged to master, it's in timholy/Revise.jl#25). It collects all methods calling Mine by following the backedges from the constructor, and then re-defines bar from the source expression. (Not all parts of that are done yet, specifically the lookup of the source expression from the method.) So in fact it will mean the new Mine, after that is defined, but it will get there by invalidating the caches on all methods that called the old Mine.

As you say, cache invalidation is hard, but here I'm hoping you've already done all the hard work and this is just gravy.

[vtjnash]

As you say, cache invalidation is hard, but here I'm hoping you've already done all the hard work and this is just gravy.

In the #265 fix, I explicitly don't handle on-stack replacement, which would be required for this. Heuristically, that approach should mostly work currently, since it does cause the system to rebuild most of the state going forward. But I don't think I'm going to keep backedges around – I think the system would be less aggressive at cache invalidation if I stored forward edges, since it can then be more precise at verifying just whether or not the method table inference result changed, and ignore any spurious changes.

I am, see the diff for base/reflection.jl.

I don't see it. Did you forget to git add it? Or is this in a gist?

[timholy]

julia/base/reflection.jl

Line 114 in eb1c523

T.name.name = newname

?

[timholy]

I'd like to revive this and therefore added a bunch of tests. I've barely tested it in real-world usage, but together with #33883 and this branch of Revise, things look fairly promising for type-renaming.

The only hint of weirdness I've seen is encapsulated here. (EDIT: and one more, constructors don't seem to store backedges, so one can't automatically discover that f() = MyType{Int}(2) needs to be re-evaluated.) @vtjnash, I confess I don't understand your concerns about ptrhash because AFAICT it's just a symbol-ptr lookup, and all I'm doing is changing the name (but not the pointer) and I fix up the hashing of the name lookup. If there are some places where the table pointer escapes, clearly that could cause a problem, but perhaps we could put some kind of lock around those uses?

[JeffBezanson]

I think it would be very hard to get this working. The issue you have is probably because we don't have backedges for bindings (i.e. the ability to invalidate code if a constant binding changes). Almost everything in the system is accessed via such bindings, so adding those edges seems very expensive to me.

[timholy]

Can you clarify what circumstances will require invalidation?

julia> struct A
           x::Int
       end

julia> foo(::A) = true
foo (generic function with 1 method)

julia> a = A(2)
A(2)

julia> foo(a)
true

julia> Base.rename_binding(Main, :A, :Aold)
Aold

julia> a
Aold(2)

julia> foo(a)
true

julia> methods(foo)
# 1 method for generic function "foo":
[1] foo(::Aold) in Main at REPL[2]:1

[JeffBezanson]

Inferred/compiled code that refers to the old binding.

[timholy]

Can you give me a concrete demo? I thought bindings were resolved at compile time. This is probably completely redundant with the tests I added, but I threw in an attempt at compiled bindings in the createA method below. Here's a package:

module HasA

export A, used, unused

struct A
   x::Int
end

used(::A) = true
unused(::A) = true

if ccall(:jl_generating_output, Cint, ()) == 1
    @assert precompile(used, (A,))
end

end

with test file

using HasA, Test

function createA()
    A = getfield(HasA, :A)
    return A(2)
end

a = createA()
@test used(a)

Base.rename_binding(HasA, :A, :Aold)

@test used(a)

anew = createA()
@test used(anew)

@test unused(a)  # not yet compiled
@test unused(anew)

Unfortunately 😄 it worked. Both a and anew claim to be of type Aold.

What I'm aiming for (but which may not be possible) is this: old objects get a new typename but in principle all their old methods continue to work---everything acts as if the new name had been used originally. No recompilation or anything is necessary. But this clears space for the user to define a new type with the old name.

That new type has no methods other than its constructor. But that's OK, and it's definitely not Base's problem. It's a user problem, or Revise's.

(RE)EDIT: to clarify, Revise will may delete methods that accept the old type as a declared argument type, to force gently pressure users to create new objects of the new types. But yes, createA will still generate old objects (I suspect this is what you're referring to, and what I was referring to as a lack of backedges to the constructors), and yes, generic methods like getx(obj) = obj.x will continue to work on old types.

I think the main question is whether users will be confused by having stale code generate and use old types. If so, I can think of several steps we could take to issue warnings about using old types (examining arguments issued from the command line, invalidating old specializations, putting a hook into inference to check for new compilations with old types, etc).

This doesn't have to be merged immediately---we could let people play with this + that Revise branch for a while and see what problems crop up. But since the merge deadline is less than a month away and people are busy writing/reviewing code, I wanted to put this forward with time to address specific concerns.

[timholy]

OK, I think there's a pretty good way forward, but I could use a little extra info. With this gist I count the number of Float64s that appear in all codeinfo.ssavluetypes in Base:

julia> tc = TypeCounter{Float64}()
TypeCounter{Float64}(0, 0, 0, 0, 0, 0, 0)

julia> @time traverse(tc, Base)
WARNING: Threads.Mutex is deprecated, use ReentrantLock instead.
  likely near REPL[5]:1
WARNING: Threads.RecursiveSpinLock is deprecated, use ReentrantLock instead.
  likely near REPL[5]:1
  0.341304 seconds (3.42 M allocations: 206.899 MiB)
TypeCounter{Float64}(46, 4212, 16174, 48884, 46417, 33002, 2086)

So in 1/3 second (which is totally reasonable, and the time is entirely due to Core.Compiler._uncompressed_ast), I can discover that there are 2086 uses of Float64 among ≈50k MethodInstances. Of course the number of distinct MethodInstances that involve Float64 may be much lower, because many methods that use one Float64 probably use multiple Float64s.

If needed, this provides a viable mechanism to discover all the methods that have cached inference results that use a particular type without needing to cache edges. My concern is those MethodInstances that don't cache their inference results. When does that happen? There are ≈50k MethodInstances but only ≈33k CodeInfos.

[timholy]

OK, serious progress. The latest version of the teh/typeredef branch of Revise still needs some serious real-world testing (TBD), but it's beginning to look like the bee's knees with only a couple of caveats:

• the bizarre need for printing: https://github.com/timholy/Revise.jl/blob/bd5f1a0b040e8dece648480e3a802f89ffbeca80/src/Revise.jl#L703-L706
• the fact that there are a good number of compiled methods that don't cache their inference. Want to know why and whether we can change that.
• the fact that the optimizer can sometimes fully elide any traces that a particular type was involved in any of its decisions. This is probably the most difficult problem, but seems likely to be solvable in several ways.

Other than those caveats, the very preliminary analysis suggests it behaves exactly as you'd expect for a 265-like fix for type redefinition. 🎆

[JeffBezanson]

One way to look at this is that currently, sort of by luck, if the compiler depends on a certain constant binding, it inlines the value and removes any reference to the binding itself. So if you want that code to keep working the same way, it works automatically. But that is basically depending on an implementation detail.

The easiest way to demonstrate the problem now might be with precompiled modules. When package B uses package A and the type A.T, references to A.T in B are stored by name. When B is loaded it looks up the needed type as T, so if T has changed to something else it will get the wrong type.

[timholy]

Ah, very helpful. FWIW the tests in Revise do exercise precompiled modules, but I first load all the modules that access the binding and then I invalidate the type. The problem you describe would occur if I did it in the opposite order.

Can I ask, why don't some methods cache inference results? Is it only the inline-worthy ones that are preserved?