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

more precise types in keyword argument method lowering #30926

Merged
merged 1 commit into from
Feb 7, 2019

Conversation

JeffBezanson
Copy link
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
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
Member Author

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

@nanosoldier
Copy link
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
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