Skip to content
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.

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

WIP: Make hasmethod able to be used for static dispatch #32732

Closed
wants to merge 9 commits into from

Conversation

oxinabox
Copy link
Contributor

@oxinabox oxinabox commented Jul 30, 2019

We can make hasmethod able to be used for static dispatch, for truely excellent traits,
by making it trigger recompilation of the all functions using it when/if the answer changes.

This is a minor extension of #32237 (in particular addressing this comment)
This will close #32704

TODO:

Very small steps / fine-grained tasks:

  • make edges connectable to MethodTables
  • Connect the edges in hasmethod
  • Workout how to write the CodeInfo that should be returned for constant false.
  • Make this play nice with delete_method
  • Make it work with Functors and Constructors
  • Test all the above

@bramtayl
Copy link
Contributor

Ref #30408

@nalimilan
Copy link
Member

I guess this supersedes #16422?

@chethega
Copy link
Contributor

I see an issue with ambiguity errors:

julia> foo(x, y::Int)=1
foo (generic function with 1 method)
julia> hasmethod(foo, Tuple{Int,Int})
true
julia> foo(x::Int, y)=2
foo (generic function with 2 methods)
julia> hasmethod(foo, Tuple{Int,Int})
false

@ararslan ararslan requested review from timholy and vtjnash July 30, 2019 23:28
@oxinabox
Copy link
Contributor Author

oxinabox commented Jul 31, 2019

This works you can paste it into your REPL, on this branch.

_hasmethod_false(@nospecialize(f), @nospecialize(t)) = false

@generated function static_hasmethod(@nospecialize(f), @nospecialize(t::Type{T})) where T<:Tuple
    fi = f.instance  #TODO: make this work with constructors and functors.
    typ = Base.signature_type(fi, T)
    Core.show(typ)    
    world = 0xffff_ffff_ffff_ffff  # can't use type-max, it hasn't been delcared yet
    method_doesnot_exist = ccall(:jl_gf_invoke_lookup, Any, (Any, UInt), typ, world) === nothing
    if method_doesnot_exist
        ci = copy(Base.uncompressed_ast(typeof(_hasmethod_false).name.mt.defs.func))

        # Now we add the edges so if a method is defined this recompiles
        mt = f.name.mt
        ci.edges = [mt, typ]
        return ci
    else
        return true  # We are done, it exists, no need to recompile ever
        # except if it is deleted. TODO: deal with Base.delete_method
    end
end


foo(::Int)=1
tell_of_having_foo(x::T) where T = static_hasmethod(foo, Tuple{T}) ? "It has it" : "It does not have it"

@show tell_of_having_foo(1)
@show tell_of_having_foo('a')
foo(::Char)=2
@show tell_of_having_foo('a')
@code_typed tell_of_having_foo('a')

The one I added to Base & Core does not work yet.
I don't yet have something that can run in that world-age / point of boot-strapping.
But I am working on it.

base/compiler/typeinfer.jl Outdated Show resolved Hide resolved
@oxinabox
Copy link
Contributor Author

julia> foo(x, y::Int)=1
foo (generic function with 1 method)
julia> hasmethod(foo, Tuple{Int,Int})
true
julia> foo(x::Int, y)=2
foo (generic function with 2 methods)
julia> hasmethod(foo, Tuple{Int,Int})
false

Yeah, that does indeed seem to be an issue.
I didn't know hasmethod returned false for ambigious cases.
(the docs don't say anything about that)

With this PR right now:

julia> foo(x, y::Int)=1
foo (generic function with 1 method)

julia> hasmethod(foo, Tuple{Int,Int})
true

julia> foo(x::Int, y)=2
foo (generic function with 2 methods)

julia> hasmethod(foo, Tuple{Int,Int})
true

Copy link
Member

@timholy timholy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I worry about how this scales. If this becomes a foundation of traits and you get 1000 methods that call hasmethod, don't you have to recompile all of them whenever you add a single method to any method table?

base/reflection.jl Show resolved Hide resolved
@oxinabox
Copy link
Contributor Author

If this becomes a foundation of traits and you get 1000 methods that call hasmethod, don't you have to recompile all of them whenever you add a single method to any method table?

Not really, at least not as i understand it.

My understanding is: this edge goes from:
this method instance of hasmethod which is unique based on the types of the agruements (i.e. the function + sig being queried about),
to basically a pending method-instance-to-be, characterised by the method table (mt) and signature (typ)
(Not to the whole method table for the function).
And the recompilation is only triggered when the method-instance-to-be comes into existance.

As i understand it it basically boils down to the same machinery that makes recompilation of things that were triggering MethodErrors before work.

julia> inner(::Int)=1
inner (generic function with 1 method)

julia> outer(x) = 2*inner(x)
outer (generic function with 1 method)

julia> outer("a")
ERROR: MethodError: no method matching inner(::String)
Closest candidates are:
  inner(::Int64) at REPL[25]:1
Stacktrace:
 [1] outer(::String) at ./REPL[26]:1
 [2] top-level scope at REPL[27]:1

julia> inner(::String)=2
inner (generic function with 2 methods)

julia> outer("a")
4

@vtjnash
Copy link
Member

vtjnash commented Aug 2, 2019

There seems like some significant understanding issues here that will sink this approach, sorry. Edges are only the half of the computation, and even so, they still mandate that the result of the generated function must never change for a given set of inputs. For this reason, quoting from https://docs.julialang.org/en/v1/manual/metaprogramming/#Generated-functions-1: "Generated functions must not...use hasmethod."

Can you make the "edges connectable to MethodTables" parts a separate PR? I see no issues with that aspect.

For applicable (a similar but simpler function that takes values), we can add a simple inference tfunc to make it inferrable with a few lines of code, if we want to (currently it's not defined because we don't want anyone to build interfaces this way).

For hasmethod (this PR), the ability to explicitly pass the world to that method makes this function return value pretty fundamentally unpredictable for inference. We would need to track the provenience of that value (e.g. know how we got it) and guarantee that it didn't come from an invalid source (for example, a constant or Val(typemax(UInt))) in order to be legal (only 1 and jl_get_world_counter() are guaranteed to be valid inputs). That'd be super weird and a pretty horrible thing to be doing during compilation time. (at runtime it's easy, because we know that the result is potentially invalid as soon as the function returns if the caller passed in a constant, and we just don't care)

@oxinabox
Copy link
Contributor Author

oxinabox commented Aug 2, 2019

For this reason, quoting from https://docs.julialang.org/en/v1/manual/metaprogramming/#Generated-functions-1: "Generated functions must not...use hasmethod."

I wrote that line 😀 . But I thought with edges I could be free of that particular rule.
I'm not sure I understand why edges don't free me from it?
I thought we always would recompile, and my (very simple) tests seems to agree.

Is the remainder because things calling generated functions might not have edges that will be triggered in turn?

For hasmethod (this PR), the ability to explicitly pass the world to that method makes this function return value pretty fundamentally unpredictable for inference.

Oh yeah, I didn't take that into account. Fair.
Can I have a version of it that always does the current world?

@bramtayl
Copy link
Contributor

bramtayl commented Aug 2, 2019

currently it's not defined because we don't want anyone to build interfaces this way

How do we want people to build interfaces?

@vtjnash
Copy link
Member

vtjnash commented Aug 3, 2019

I'm not sure I understand why edges don't free me from it?

Adding edges don't change the strong requirement that the result of the generated function must be independent of all state (such as which methods exist) given the inputs (types). Nor is it even the only thing you need to track (you also need to track the effect on the age validity range, for example). The problem is that world=typemax(UInt) isn't actually a constant value (it's essentially of type Ref{MethodTable}, but that would look—and act—weird so I don't make it actually look like a pointer). Thus you can't safely observe it until runtime (unless you know where it came from—which is how inference is able to use it safely).

Can I have a version of it that always does the current world?

Yeah, that's how we defined applicable

@vtjnash
Copy link
Member

vtjnash commented Aug 3, 2019

How do we want people to build interfaces?

duck typing

@NHDaly
Copy link
Member

NHDaly commented Aug 3, 2019

Adding edges don't change the strong requirement that the result of the generated function must be independent of all state (such as which methods exist) given the inputs (types). Nor is it even the only thing you need to track (you also need to track the effect on the age validity range, for example).

I've just opened #32774, which @oxinabox, @vchuravy and I worked on together at the hackathon last week. I think that this conversation will be relevant there as well.

If there are other things besides edges that make this kind of recompilation still not work, can we think about fixing those as well? If we got that PR to work, does the requirement that these functions don't rely on mutable state like method tables still apply? I would think that this would be enough to lift that restriction?

Thanks for your help, Jameson! :)

@vtjnash
Copy link
Member

vtjnash commented Aug 3, 2019

As it seems like you note in the tests over there, that approach won't work. I think it's theoretically possible to you could write a pure JuliaInterpreter which only permits side-effects that it can fully reason about and returns the set of operations ("edges") that describe how the answer could be changed by new method definitions. If I had to guess, that's a fairly large project though.

@StefanKarpinski
Copy link
Member

@vtjnash, for the rest of us who aren't quite as fully suffused with this stuff as you, could you write a bit more about why this is impossible?

@oxinabox
Copy link
Contributor Author

oxinabox commented Aug 9, 2019

I now understand why this won't work.
Thanks to @NHDaly who is going to write up an explanation.

The short version is: if not fully type inferred then it will become a dynamic dispatch at the call-site. Dynamic dispatches don't need to (and thus don't) set edges. (I suspect this is a bit of a simplification, waiting for full version.)

The actual case I care about for traits though always fully infers, since everything is eiter literal types or the results of typeof.
So the question becomes how can I expose that demand on the user.
I think maybe a macro?

@NHDaly
Copy link
Member

NHDaly commented Aug 13, 2019

(the full writeup is here: #32774 (comment), and applied to the problematic example, here: #32774 (comment))

@oxinabox
Copy link
Contributor Author

oxinabox commented Sep 1, 2019

This plan in its current form isn't going to work out.
I am going to close this and open a PR with a new design, with tfuncs

For now the code is in Tricks.jl

@clarkevans
Copy link
Member

@oxinabox Did you open a new design for static_hasmethod?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add ability to attach edges from MethodInstances to MethodTables
10 participants