Skip to content

more precise types in keyword argument method lowering#30926

Merged
JeffBezanson merged 1 commit into
masterfrom
jb/kwmethodsigs
Feb 7, 2019
Merged

more precise types in keyword argument method lowering#30926
JeffBezanson merged 1 commit into
masterfrom
jb/kwmethodsigs

Conversation

@JeffBezanson
Copy link
Copy Markdown
Member

We have a bad interaction between two features: (1) generated methods for keyword arguments need to pass the original function around in case it contains closure data, (2) functions that take a function argument but don't call it aren't specialized on that argument (to avoid excessive specialization). That can lead to slow calls in keyword method shims, but is a bit unpredictable. For example it caused the slowdown observed in #30830 (comment), and likely other intermittent performance changes we've seen.

Long story short, this is before:

julia> code_typed(string, (Int,))
1-element Array{Any,1}:
 CodeInfo(
1 ─ %1 = invoke Base.:(#string#322)(10::Int64, 1::Int64, _1::Function, _2::Int64)::String
└──      return %1
) => String

and this is after:

julia> code_typed(string, (Int,))
1-element Array{Any,1}:
 CodeInfo(
1 ─ %1 = invoke Base.:(#string#322)(10::Int64, 1::Int64, _1::typeof(string), _2::Int64)::String
└──      return %1
) => String

Now the front end puts a specific type on the argument slot used to forward the function. After all, it knows exactly what function we're dealing with...right? Well, it can be quite difficult to do for closures, where we need to ensure everything is defined in just the right order, since there are some circular dependencies (method A calls method B, but method B needs to mention A in its signature). I hope this works.

@JeffBezanson JeffBezanson added performance Must go faster compiler:lowering Syntax lowering (compiler front end, 2nd stage) keyword arguments f(x; keyword=arguments) labels Jan 31, 2019
@timholy
Copy link
Copy Markdown
Member

timholy commented Jan 31, 2019

Having poked at these recently, I wondered about precisely this change, but I didn't understand where the difficulties and opportunities lay. Specifically, I had wondered about adding the type info so that one could guess where the #self# argument was, but then came to the conclusion that it would be defeated by nefarious methods like foo(x; f::typeof(foo)=foo) (which seems pointless and cruel). This isn't currently allowed due to circular dependencies, however.

@JeffBezanson JeffBezanson added the needs nanosoldier run This PR should have benchmarks run on it label Feb 1, 2019
@JeffBezanson
Copy link
Copy Markdown
Member Author

@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 @ararslan

@JeffBezanson JeffBezanson removed the needs nanosoldier run This PR should have benchmarks run on it label Feb 1, 2019
@JeffBezanson JeffBezanson force-pushed the jb/kwmethodsigs branch 4 times, most recently from 1cd1e6c to 75e9b02 Compare February 5, 2019 20:17
@JeffBezanson JeffBezanson added the DO NOT MERGE Do not merge this PR! label Feb 5, 2019
@JeffBezanson
Copy link
Copy Markdown
Member Author

I ran NewPkgEval and identified some issues. Tests added. Also needs #30972, then this should be ready to go.

@JeffBezanson JeffBezanson removed the DO NOT MERGE Do not merge this PR! label Feb 7, 2019
@JeffBezanson JeffBezanson merged commit c6c3d72 into master Feb 7, 2019
@JeffBezanson JeffBezanson deleted the jb/kwmethodsigs branch February 7, 2019 17:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

compiler:lowering Syntax lowering (compiler front end, 2nd stage) keyword arguments f(x; keyword=arguments) performance Must go faster

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants