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

support passing a specific Method to invoke #56692

Merged
merged 2 commits into from
Nov 28, 2024
Merged

Conversation

vtjnash
Copy link
Member

@vtjnash vtjnash commented Nov 27, 2024

The main purpose of this is integrating with external compilers using overlay method tables, where the Method might need to come from a source other than regular dispatch. Note that this is generally much less well optimized at runtime than generic dispatch, so this shouldn't be treated as expecting to give a performance boost in any dynamic cases.

Trivial examples:

julia> let m = which(+, (Int, Int))
       @eval f(i, j) = invoke(+, $m, i, j)
       end
julia> f(2,2)
4

julia> let m = which(Core.kwcall, (@NamedTuple{base::Int}, typeof(string), Int))
       @eval f(i, base) = @noinline invoke(string, $m, i; base)
       end
julia> f(20,16)
"14"

@vtjnash vtjnash added the needs tests Unit tests are required for this change label Nov 27, 2024
@vtjnash vtjnash requested a review from Keno November 27, 2024 00:32
src/builtins.c Show resolved Hide resolved
base/docs/basedocs.jl Outdated Show resolved Hide resolved
@@ -185,6 +185,11 @@ function Core.kwcall(kwargs::NamedTuple, ::typeof(invoke), f, T, args...)
T = rewrap_unionall(Tuple{Core.Typeof(kwargs), Core.Typeof(f), (unwrap_unionall(T)::DataType).parameters...}, T)
return invoke(Core.kwcall, T, kwargs, f, args...)
end
# for m::Method however, assume the user already looked it up correctly for kwargs but just needs the syntax rewrite to swap the positions of the invoke, kwcall, and other arguments
function Core.kwcall(kwargs::NamedTuple, ::typeof(invoke), f, m::Method, args...)
Copy link
Member

Choose a reason for hiding this comment

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

I don't know that I like this. If you already have to do the Method lookup yourself using the transformed args, it seems fair to require you to transform the args themselves also.

Copy link
Member Author

Choose a reason for hiding this comment

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

That is a good point. I think this can be a separate PR if needed, since it isn't part of the core functionality of the PR. I guess it mostly brings up the question: why isn't there an API for looking up the Method without doing the argument transform yourself (which requires knowing very specific and unstable details about the lookup transform--refs Base.bodyfunction?), and should there be one?

@tecosaur
Copy link
Contributor

This is rather interesting to see. If you wouldn't mind a question, I've been identifying the Methods I want to call via reflection with methods(...) in a package of mine, effectively doing multi-function dispatch. After identifying the methods I want to call, it's seemed like a pity that I couldn't just directly invoke them.

While seemingly not part of the intended purpose of this PR, how well does that sort of use fit with this feature?

@vtjnash
Copy link
Member Author

vtjnash commented Nov 27, 2024

After identifying the methods I want to call, it's seemed like a pity that I couldn't just directly invoke them. While seemingly not part of the intended purpose of this PR, how well does that sort of use fit with this feature?

Doing what you are doing is guaranteed to be slower than doing normal dynamic dispatch, as it has fewer optimizations possible for it, but yes you could do that using this

The main purpose of this is integrating with external compilers using
overlay method tables, where the Method might need to come from a source
other than regular dispatch. Note that this is generally much less well
optimized at runtime than generic dispatch, so this shouldn't be treated
as expecting to give a performance boost in any dynamic cases.

Trivial examples:

    julia> let m = which(+, (Int, Int))
           @eval f(i, j) = invoke(+, $m, i, j)
           end
    julia> f(2,2)
    4

    julia> let m = which(Core.kwcall, (@NamedTuple{base::Int}, typeof(string), Int))
           @eval f(i, base) = invoke(Core.kwcall, $m, (;base), string, i)
           end
    julia> f(20,16)
    "14"
@vtjnash vtjnash added merge me PR is reviewed. Merge when all tests are passing and removed needs tests Unit tests are required for this change labels Nov 27, 2024
NEWS.md Outdated Show resolved Hide resolved
Co-authored-by: Mosè Giordano <[email protected]>
@IanButterworth IanButterworth merged commit 79d8d3f into master Nov 28, 2024
7 checks passed
@IanButterworth IanButterworth deleted the jn/invoke-a-method branch November 28, 2024 03:13
@giordano giordano removed the merge me PR is reviewed. Merge when all tests are passing label Nov 28, 2024
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.

5 participants