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

make default value of rev keyword argument type stable #24422

Merged
merged 1 commit into from
Nov 1, 2017
Merged

Conversation

JeffBezanson
Copy link
Sponsor Member

@JeffBezanson JeffBezanson commented Oct 31, 2017

This lets us remove the method overwrite during sysimg build, and allows us to fix #9498. There's no user-visible change except that rev=nothing will now work (though it's undocumented), which seems harmless enough to me. We can easily remove this later if constant propagation permits.

this lets us remove the method overwrite during sysimg build
@vtjnash
Copy link
Sponsor Member

vtjnash commented Oct 31, 2017

We can easily remove this later if constant propagation permits

How well does #24362 do towards this eventual goal?

@JeffBezanson
Copy link
Sponsor Member Author

Just tried it; it works.

The change from branches to dispatch here is a small additional help since it allows more inlining.

@JeffBezanson
Copy link
Sponsor Member Author

Oops, I didn't measure carefully enough. I thought I was looking for allocations but actually the only issue here is dispatch time. Looking again, #24362 doesn't seem to help. I get this on that branch:

julia> f() = Base.Sort.ord(isless,identity,false,Base.Order.Forward)
f (generic function with 2 methods)

julia> @code_typed f()
CodeInfo(:(begin
        return $(Expr(:invoke, MethodInstance for ord(::typeof(isless), ::typeof(identity), ::Bool, ::Base.Order.ForwardOrdering), :($(QuoteNode(Base.Order.ord))), :(Main.isless), :(Main.identity), false, :($(QuoteNode(Base.Order.ForwardOrdering())))))
    end)) => Any

@JeffBezanson
Copy link
Sponsor Member Author

Aha, I suspect all that's needed there is more precise t-functions for and_int, or_int, and not_int on Bool.

@JeffBezanson
Copy link
Sponsor Member Author

Ok, that's still not quite enough. This is a case where we need to actually call or inline the code specialized for false.

@StefanKarpinski
Copy link
Sponsor Member

Specialization on default values would presumably also have a similar effect, no?

@vtjnash
Copy link
Sponsor Member

vtjnash commented Oct 31, 2017

inline the code specialized for false
more precise t-functions

Ah, of course. The two next PRs I had planned. (noted briefly in the todo note in the initial PR).

@JeffBezanson JeffBezanson merged commit 4b06266 into master Nov 1, 2017
@JeffBezanson JeffBezanson deleted the jb/revkw branch November 1, 2017 21:30
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.

Keyword arguments affect methods dispatch
3 participants