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

improve method specificity algorithm #22162

Merged
merged 2 commits into from
Jun 5, 2017
Merged

improve method specificity algorithm #22162

merged 2 commits into from
Jun 5, 2017

Conversation

JeffBezanson
Copy link
Member

This code needed some weeding and strengthening. First, I had the existing code write out a trace of all non-subtype specificity results computed during the system image build (45MB, and took so long just to parse that it prompted me to do #22161). Then I added assertions to ensure that morespecific(a,b) => !morespecific(b, a) during the algorithm, and otherwise tried to clean up the logic as much as I could. I tested that against the trace, fixed the cases that needed fixing, and extracted interesting cases into a new specificity test suite.

This makes many changes to the specificity relation, but mostly involving disjoint types, so it does not seem to affect any code behavior, at least in the test suite. However it is very hard to know whether this will break anything.

@JeffBezanson JeffBezanson added the types and dispatch Types, subtyping and method dispatch label May 31, 2017
@timholy
Copy link
Member

timholy commented May 31, 2017

Thanks! Perhaps check if it fixes issues #22002, #20995, #20714, #20672, #17278, #10174, #6383, #3025.

@vtjnash
Copy link
Member

vtjnash commented May 31, 2017

Could we please get rid of all variable names with < 6 characters in this file? I hate to impose some sort of arbitrary limit, but I've been having trouble following along with the recent PRs to this file since I can't keep straight what these acronyms mean. The aggressive compression of variable names in this PR (env -> e, akind -> ak, for example) is only making it worse. (a little more whitespace, so the structure of the code is visible at a glance rather than requiring actual parsing of the code, would also be much appreciated 🙂)

@JeffBezanson
Copy link
Member Author

Can I keep i?

@JeffBezanson
Copy link
Member Author

I will also have trouble thinking of > 6 character names for a, b, and n.

@vtjnash
Copy link
Member

vtjnash commented May 31, 2017

yes, you can keep i :)

@JeffBezanson
Copy link
Member Author

Ok I tried to do a few style improvements. Most importantly I think the tuple specificity algorithm itself is now far simpler.

@JeffBezanson
Copy link
Member Author

@timholy I didn't expect much, but lo and behold this does fix #22002! That might have been a similar issue to #22164. Thanks for the very good test case there.

@timholy
Copy link
Member

timholy commented Jun 1, 2017

Woot! Thanks for checking (always nice to add to the "this closes n issues" stats 😉). And of course I'm especially grateful for the fix; convert is particularly scary when it comes to specificity, because there are so many methods, so it's awesome that you got it on the way to other things.

simplify tuple type specificity implementation

fix specificity `A{B,B,C} < A{B,C,D}`
@JeffBezanson JeffBezanson merged commit a593118 into master Jun 5, 2017
@JeffBezanson JeffBezanson deleted the jb/specificity branch June 5, 2017 20:55
tkelman pushed a commit that referenced this pull request Jun 5, 2017
fix #22164, specificity issue in ForwardDiff

(cherry picked from commit e92eb58)
partial backport of #22162
@timholy
Copy link
Member

timholy commented Jun 6, 2017

Is this backportable?

@JeffBezanson
Copy link
Member Author

The fix for #22002 was backported in #22206.

JeffBezanson added a commit that referenced this pull request Jun 15, 2017
simplify tuple type specificity implementation

fix specificity `A{B,B,C} < A{B,C,D}`

Backported from PR #22162

(cherry picked from commit 4634a3d)

Conflicts:
	test/core.jl
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
types and dispatch Types, subtyping and method dispatch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants