Skip to content

add Type to kwargs internal argument#39593

Merged
vtjnash merged 1 commit intomasterfrom
jn/39419
Mar 2, 2021
Merged

add Type to kwargs internal argument#39593
vtjnash merged 1 commit intomasterfrom
jn/39419

Conversation

@vtjnash
Copy link
Copy Markdown
Member

@vtjnash vtjnash commented Feb 10, 2021

Fixes #39419

# No dependencies
:ArgTools,
:Artifacts,
:ArgTools,
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Out of curiosity, why reorder? (You successfully nerd sniped me 😄.)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

oops, that wasn't for this PR (I was timing ArgTools, and wondering why it was one of the slower packages to load)

@timholy
Copy link
Copy Markdown
Member

timholy commented Feb 10, 2021

Awesome, thanks so much! While you're at it, maybe change

k = k::Symbol

to k = Symbol(k)? While I think the typeassert was practically the right thing to do at the time, it's a weird limitation for an exported, generic function like merge and where itr can be anything. (Presumably we should support any keys from which a Symbol can be created, or at least those which can convert to a Symbol.)

If you're at all worried about the possibility of invalidation regressions, you can check the invalidations from PyPlot. I'm sure you've done this before many times, perhaps using more low-level tools, but in case it saves you any time here's a recipe of how I'd do this:

julia> using SnoopCompileCore

julia> invs = @snoopr using PyPlot;

julia> using SnoopCompile

julia> length(uinvalidated(invs))
1220

😱 Something has already regressed! Looks like 8e61551.

So the bar is low for this particular change, get it in before others raise the standards! 🙂

Pairs{K, V}(data::A, itr::I) where {K, V, I, A} = Pairs{K, V, I, A}(data, itr)
Pairs{K}(data::A, itr::I) where {K, I, A} = Pairs{K, eltype(A), I, A}(data, itr)
Pairs(data::A, itr::I) where {I, A} = Pairs{eltype(I), eltype(A), I, A}(data, itr)
pairs(::Type{NamedTuple}) = Pairs{Symbol, V, NTuple{N, Symbol}, NamedTuple{names, T}} where {V, N, names, T<:NTuple{N, Any}}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This seems a bit strange to have as a method; maybe just make it a constant instead?

sparams))
(kw (gensy))
(rkw (if (null? restkw) (make-ssavalue) (symbol (string (car restkw) "..."))))
(restkw (map (lambda (v) `(|::| ,v (call (top pairs) (core NamedTuple)))) restkw))
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This could be merged with the first definition of restkw.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

computing rkw needs the initial form (to stringify the variable name) before this wrapper is applied

@JeffBezanson
Copy link
Copy Markdown
Member

to k = Symbol(k)?

That would be a dramatic change, since we allow constructing a Symbol from anything by stringification.

@timholy
Copy link
Copy Markdown
Member

timholy commented Feb 10, 2021

Oh, funny I didn't know that. convert(Symbol, k) then?

@JeffBezanson
Copy link
Copy Markdown
Member

Have you come across a use case for it? I'm surprised to see the Invalidation Czar himself propose this :)

@timholy
Copy link
Copy Markdown
Member

timholy commented Feb 11, 2021

Nah. For me Symbols live in their own world; until just now I also hadn't really that there are no objects in Base that convert to Symbol:

julia> methods(convert, (Type{Symbol}, Any))
# 1 method for generic function "convert":
[1] convert(::Type{T}, x::T) where T in Base at essentials.jl:171

All this explains why that change passed tests and hasn't caused other known problems. I was just motivated by the fact that it looks like a generic method, even if nothing takes advantage of that. If nothing else we could document that the keys must be Symbol.

@vtjnash vtjnash added the needs nanosoldier run This PR should have benchmarks run on it label Feb 12, 2021
@vtjnash
Copy link
Copy Markdown
Member Author

vtjnash commented Feb 26, 2021

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

@nanosoldier
Copy link
Copy Markdown
Collaborator

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

@vtjnash vtjnash removed the needs nanosoldier run This PR should have benchmarks run on it label Mar 2, 2021
@vtjnash vtjnash merged commit 49387b2 into master Mar 2, 2021
@vtjnash vtjnash deleted the jn/39419 branch March 2, 2021 21:03
@timholy
Copy link
Copy Markdown
Member

timholy commented Mar 2, 2021

Thanks again, @vtjnash!

maleadt referenced this pull request Mar 3, 2021
Generally we assume parameters can be duplicated without seeing
side-effects. That is not entirely true of mutable globals and
multi-threading.

Refs: #36450
Fixes: #39508
ElOceanografo pushed a commit to ElOceanografo/julia that referenced this pull request May 4, 2021
simeonschaub added a commit to simeonschaub/Revise.jl that referenced this pull request May 5, 2021
since JuliaLang/julia#39593, this statement was not working anymore
antoine-levitt pushed a commit to antoine-levitt/julia that referenced this pull request May 9, 2021
KristofferC pushed a commit to timholy/Revise.jl that referenced this pull request May 10, 2021
since JuliaLang/julia#39593, this statement was not working anymore
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.

Lower keyword-body methods with tighter signature

5 participants