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

use named tuples as keyword varargs. fixes #4916 #24795

Merged
merged 1 commit into from
Dec 3, 2017
Merged

Conversation

JeffBezanson
Copy link
Member

Same highly-biased benchmark as #21915 (~100x speedup):

julia> f(;kw...) = g(;kw...);

julia> g(;kw...) = kw;

julia> c = g(a=1,b=2);

julia> @time for i = 1:10000; f(a=1,b=2); end
  0.000010 seconds

julia> @time for i = 1:10000; f(;c...); end
  0.000577 seconds (20.00 k allocations: 625.000 KiB)

This also helps a lot towards #9551. The PR makes the lowering of keyword argument methods simpler and easier to optimize (note the net code deletion). In fact I think just a bit of constant propagation (#24362) on top of this will solve the problem entirely.

Also fixes #9972.

@JeffBezanson JeffBezanson added the keyword arguments f(x; keyword=arguments) label Nov 26, 2017
NEWS.md Outdated
* Keyword argument containers (`kw` in `f(; kw...)`) are now named tuples. Dictionary
functions like `haskey` and indexing can be used on them, and name-value pairs can be
iterated using `pairs(kw)` ([#4916]).

Copy link
Member

Choose a reason for hiding this comment

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

I wonder if it'd be worth mentioning that keyword arguments can no longer be repeated; I know that wasn't changed explicitly in this PR, but seems like a good place to mention it anyway.

@vtjnash
Copy link
Member

vtjnash commented Nov 26, 2017

What constant propagation would that be? Currently that PR can’t really do anything for NamedTuples (which would likely need more of a MemSSA or GVN pass).

@JeffBezanson
Copy link
Member Author

The code here calls haskey to see whether a keyword argument was passed or should get its default value. If we can remove the branch then code with keyword arguments can be fully inferred. In fact it already works in many cases, if the passed value has the same type as the default value.

@JeffBezanson JeffBezanson merged commit 04ae93a into master Dec 3, 2017
@JeffBezanson JeffBezanson deleted the jb/kwargs branch December 3, 2017 16:46
c42f added a commit to c42f/julia that referenced this pull request Dec 3, 2017
c42f added a commit to c42f/julia that referenced this pull request Dec 3, 2017
fredrikekre added a commit to fredrikekre/julia that referenced this pull request Dec 4, 2017
StefanKarpinski added a commit that referenced this pull request Dec 5, 2017
fix deprecation of IOContext after #24795
@ChrisRackauckas
Copy link
Member

What's the timing now that #24362 merged?

@yuyichao
Copy link
Contributor

yuyichao commented Dec 8, 2017

It's not measuring anything...

julia> f(;kw...) = g(;kw...)
f (generic function with 1 method)

julia> g(;kw...) = kw
g (generic function with 1 method)

julia> const c = g(a=1, b=2)
(a = 1, b = 2)

julia> @time for i in 1:1000; f(a=1, b=2); end
  0.000003 seconds

julia> @time for i in 1:1000; f(;c...); end
  0.000003 seconds

evetion pushed a commit to evetion/julia that referenced this pull request Dec 12, 2017
vtjnash added a commit that referenced this pull request Dec 27, 2017
This starts to decouple the performance improvement of #24795
from the existence of exactly one implementation of Core.NamedTuple,
in preparation for implementing NamedTuple in Julia rather than C.
Keno pushed a commit that referenced this pull request Jun 5, 2024
This starts to decouple the performance improvement of #24795
from the existence of exactly one implementation of Core.NamedTuple,
in preparation for implementing NamedTuple in Julia rather than C.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
keyword arguments f(x; keyword=arguments) performance Must go faster
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Better error needed for misuse of keyword args
5 participants