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

decouple kwargs performance improvement from API changes #25290

Merged
merged 2 commits into from
Dec 31, 2017
Merged

Conversation

vtjnash
Copy link
Member

@vtjnash vtjnash commented Dec 27, 2017

This undoes the breaking API change introduced by #24795 (fixing #25177), while keeping the performance / inference improvement. This lets us develop the kwargs API independent of Core.NT, for example, to provide proper deprecations for future changes, specific error messages, and work towards possible future development of NT (for example, implementing it as just another normal type in Julia, rather than requiring a special cases in the runtime).

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.
@ararslan ararslan added the keyword arguments f(x; keyword=arguments) label Dec 27, 2017
@@ -57,6 +57,7 @@ include("reduce.jl")
include("bitarray.jl")
include("bitset.jl")
include("abstractdict.jl")
include("iterators.jl")
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we should move IndexValue to its own file?

Copy link
Member Author

Choose a reason for hiding this comment

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

We could, but seemed useful enough to be able to call pairs (required Zip) and generator Filter and we may want to use the Product iterator for Cartesian arrays, so I’ve wanted this file before anyways.

@JeffBezanson
Copy link
Member

I think this is fine, for the sake of fixing #25177. That covers the most noticeable API change, but there are still other breaking API changes --- kwargs not being a vector --- but I think those are fine and should stay. I just want to be clear that the performance and API changes are not completely de-coupled.

@bramtayl
Copy link
Contributor

bramtayl commented Dec 31, 2017

I've updated https://github.com/bramtayl/Keys.jl master for 0.7 to take advantage of dot overloading and constant progogation (it also requires the master version of RecurUnroll). It provides a named tuple implementation that

  • Is fully type-stable
  • Requires no generated functions
  • Requires no compiler modifications
  • No macros
  • Requires no Vals
  • Allows for constant propagation (unlike the base implementation of named tuples). This seems essential making keyword arguments useful.

@vtjnash vtjnash merged commit fae72aa into master Dec 31, 2017
@vtjnash vtjnash deleted the jn/kwarg_pairs branch December 31, 2017 20:29
@vtjnash
Copy link
Member Author

vtjnash commented Dec 31, 2017

That seems interesting. What do you mean by "allows constant propagation"?

@bramtayl
Copy link
Contributor

Hmm, wait, maybe I was confused about that.

I was looking at

test() = KeyedTuple(; a = 1, b = 2.0).a
@code_warntype test()
# return 1

But this seems to work with named tuples as well:

test() = (a = 1, b = 2.0).a
@code_warntype test()
# return 1

I thought I had read somewhere that constant propagation doesn't survive named tuples?

@bramtayl
Copy link
Contributor

#25188 (comment)

@vtjnash
Copy link
Member Author

vtjnash commented Dec 31, 2017

I thought I had read somewhere that constant propagation doesn't survive named tuples?

Here's an example of that:

test() = (a = 1, b = 2.0).a + 1

@bramtayl
Copy link
Contributor

Hmm, I see basically identical generated code to

test() = KeyedTuple(; a = 1, b = 2.0).a + 1

It seems like it propagates up to

SSAValue(67) = (Base.add_int)(1, 1)::Int64
return SSAValue(67)

`(block
,(scopenest
keynames
(map (lambda (v dflt)
(let* ((k (decl-var v))
(rval0 `(call (top getfield) ,kw (quote ,k)))
(rval0 `(call (top getindex) ,kw (inert ,k)))
Copy link
Member

Choose a reason for hiding this comment

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

Will constant prop work here? I thought it was only enabled for getproperty currently.

ararslan added a commit that referenced this pull request Jan 23, 2018
Revert "undo breaking change to kwargs iteration order" (#25290)
This reverts commit b90274e.
ararslan added a commit that referenced this pull request Jan 23, 2018
Revert "undo breaking change to kwargs iteration order" (#25290)
This reverts commit b90274e.
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)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants