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

remove kwfuncs from Julia #47157

Merged
merged 3 commits into from
Oct 16, 2022
Merged

remove kwfuncs from Julia #47157

merged 3 commits into from
Oct 16, 2022

Conversation

vtjnash
Copy link
Member

@vtjnash vtjnash commented Oct 13, 2022

This is an internals-only change. These kwsorter methods only exist for dispatch, which means they do not need to be unique. We can optionally have the primary MethodTable contain an extra kwtable::MethodTable field, if this turns out to be slow, since this already introduces the concept of jl_kwmethod_table_for (for better reflection and max_args).

Plus removing this means that we can get wonderful results for invoke now, where previously this would have been broken and non-optimized:

julia> f(x...; kwargs...) = kwargs;

julia> invoke(f, Tuple, stdin=Pipe(), 1)
pairs(::NamedTuple) with 1 entry:
  :stdin => Pipe(RawFD(4294967295) init => RawFD(4294967295) init, 0 bytes waiting)

@aviatesk
Copy link
Member

@nanosoldier runbenchmarks("inference", vs=":master")

@aviatesk
Copy link
Member

Maybe REPLCompletions also need some tweak? I remember the keyword completion heavily depends on the internal implementations of keyword sorter.

@nanosoldier
Copy link
Collaborator

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here.

@vtjnash
Copy link
Member Author

vtjnash commented Oct 14, 2022

@nanosoldier runbenchmarks(!"scalar", vs=":master")

I deleted that code from REPLCompletions somewhat recently, since it gave almost exclusively wrong answers.

@nanosoldier
Copy link
Collaborator

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here.

@aviatesk
Copy link
Member

@nanosoldier runbenchmarks("misc" || "sparse" || "tuple", vs=":master")

@nanosoldier
Copy link
Collaborator

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here.

@aviatesk
Copy link
Member

@nanosoldier runbenchmarks("misc" || "sparse" || "tuple", vs=":master")

@nanosoldier
Copy link
Collaborator

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here.

@timholy
Copy link
Member

timholy commented Oct 14, 2022

I can confirm that rebasing our "replace dump.c with staticdata.c" branch (teh/make_backref_just_visited) on this one passes the 1.8-era precompile tests.

@vchuravy
Copy link
Member

@nanosoldier runtests(ALL, vs = ":master")

These kwsorter methods only exist for dispatch, which means they do not
need to be unique. We can optionally have the primary MethodTable
contain an extra kwtable::MethodTable field, if this turns out to be
slow, since this already introduces the concept of
`jl_kwmethod_table_for` (for better reflection and max_args).
This instantly grants total inference and inlining support, where
previously this was a completely opaque call!
@nanosoldier
Copy link
Collaborator

Your package evaluation job has completed - possible new issues were detected. A full report can be found here.

@vtjnash
Copy link
Member Author

vtjnash commented Oct 15, 2022

ExprTools, MethodAnalysis, and ImageView need a closer look, but probably will be okay with this, since everything else seemed fine

@timholy
Copy link
Member

timholy commented Oct 15, 2022

2 of the 3 are mine and I'm fine with fixing them. In the case of ImageView, the problematic code is in fact a workaround for #45050. If I understand correctly, it's very likely that this PR fixes that issue. So the failure on ImageView may in fact be the very opposite of a problem 🙂 .

I'd say merge when ready.

@vtjnash
Copy link
Member Author

vtjnash commented Oct 15, 2022

Okay, it looks like the other package was just using the deprecated parameter here and seems easily fixable also: invenia/ExprTools.jl#36

The issue #45050 is already apparently fixed, and wouldn't have been affected by this (since it was a lowering issue, and this is a reflection change)

vtjnash added a commit to vtjnash/MethodAnalysis.jl that referenced this pull request Oct 15, 2022
timholy pushed a commit to timholy/MethodAnalysis.jl that referenced this pull request Oct 15, 2022
* Check kwfunc method list more carefully

Attempting to prepare for JuliaLang/julia#47157
@timholy timholy merged commit ccb0a02 into master Oct 16, 2022
@timholy timholy deleted the jn/no-more-kwfuncs branch October 16, 2022 08:00
@BioTurboNick
Copy link
Contributor

Related to this PR, I'm seeing kwcall frames show up in stacktraces now.

Will they be displayed differently eventually?

image

@vtjnash
Copy link
Member Author

vtjnash commented Oct 22, 2022

I had been seeing that too, and meaning to figure out how to reproduce it. It seems my mistake is here:

diff --git a/base/errorshow.jl b/base/errorshow.jl
index 16190d64e0..65d761aefe 100644
--- a/base/errorshow.jl
+++ b/base/errorshow.jl
@@ -850,10 +850,8 @@ function process_backtrace(t::Vector, limit::Int=typemax(Int); skipC = true)
             code = lkup.linfo
             if code isa MethodInstance
                 def = code.def
-                if def isa Method
-                    if def.name === :kwcall && def.module === Core
-                        continue
-                    end
+                if def isa Method && def.sig <: Tuple{typeof(Core.kwcall),Vararg}
+                    continue
                 end
             elseif !lkup.from_c
                 lkup.func === :kwcall && continue

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.

7 participants