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

Overload resolution uses quadratic algorithm to find the best candidate. #13685

Closed
gafter opened this issue Sep 8, 2016 · 4 comments
Closed
Assignees
Labels
Area-Compilers Bug Community The pull request was submitted by a contributor who is not a Microsoft employee. Language-C# Resolution-Fixed The bug has been fixed and/or the requested behavior has been implemented Tenet-Performance Regression in measured performance of the product from goals.
Milestone

Comments

@gafter
Copy link
Member

gafter commented Sep 8, 2016

Overload resolution (unary operators, binary operators, and methods) uses a quadratic algorithm to select the best candidate, because it compares every candidate to every other candidate.

There is a simple linear algorithm that gives the same result: scan once to find a possibly best method, and then scan a second time to ensure that it is better than every other candidate.

@gafter gafter added Bug help wanted The issue is "up for grabs" - add a comment if you are interested in working on it Language-C# Area-Compilers Tenet-Performance Regression in measured performance of the product from goals. labels Sep 8, 2016
@gafter gafter added this to the 2.1 milestone Sep 8, 2016
@bbarry
Copy link

bbarry commented Sep 8, 2016

so here:

could be:

var candidates = result.Results;
if (candidates.Count < 2) return;
var best = 0;
for (; best < candidates.Count; ++best)
{
    if (candidates[best].Kind == OperatorAnalysisResultKind.Applicable)
    {
        break;
    }
}
for (var i = best + 1; i < candidates.Count; ++i)
{
    if (candidates[i].Kind != OperatorAnalysisResultKind.Applicable)
    {
        continue;
    }

    var better = BetterOperator(candidates[best].Signature, candidates[i].Signature, left, right, ref useSiteDiagnostics);
    if (better == BetterResult.Left)
    {
        candidates[i] = candidates[i].Worse();
    }
    else if (better == BetterResult.Right)
    {
        candidates[best] = candidates[best].Worse();
        best = i;
    }
}
for (var i = 0; i < candidates.Count; ++i)
{
    if (i == best)
    {
        continue;
    }
    if (candidates[i].Kind != OperatorAnalysisResultKind.Applicable)
    {
        continue;
    }

    //is it safe to simply:
    //candidates[i] = candidates[i].Worse();
    //or should this be done:
    var better = BetterOperator(candidates[best].Signature, candidates[i].Signature, left, right, ref useSiteDiagnostics);
    if (better == BetterResult.Left)
    {
        candidates[i] = candidates[i].Worse();
    }
    else if (better == BetterResult.Right)
    {
        // panic?
    }
}

I think this is potentially going to provide a different result.

If you have 3 options, A, B and C and BetterOperator would provide the results:

op1 op2 better
A    B     neither
A    C     A
B    C     C

The current implementation would leave A as the best, but this linear one would never check B vs C so both A and B would still be Applicable.

Is such an event not possible based on the spec (if A vs B is undecided then A vs C and B vs C is either always C or never C or undecided, but never mixed)?

@gafter
Copy link
Member Author

gafter commented Sep 8, 2016

@bbarry Good questions.

The same (as existing) result can be produced with a linear algorithm. Just not the one you provided.

@bbarry
Copy link

bbarry commented Sep 9, 2016

I've created this console app tool to attempt to validate the algorithm to be used against the existing one (which in its current form demonstrates the above code is not a valid replacement):

https://gist.github.com/bbarry/e78dbfec6a9f71d428e92fc37f567572

@gafter gafter added Community The pull request was submitted by a contributor who is not a Microsoft employee. and removed help wanted The issue is "up for grabs" - add a comment if you are interested in working on it labels Dec 12, 2016
@jaredpar jaredpar modified the milestones: 15.3, 15.1 Mar 9, 2017
@jaredpar jaredpar modified the milestones: 15.6, 15.3 May 30, 2017
@cston
Copy link
Member

cston commented Jun 21, 2017

Fixed in #15839.

@cston cston closed this as completed Jun 21, 2017
@sharwell sharwell added the Resolution-Fixed The bug has been fixed and/or the requested behavior has been implemented label Jun 21, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Compilers Bug Community The pull request was submitted by a contributor who is not a Microsoft employee. Language-C# Resolution-Fixed The bug has been fixed and/or the requested behavior has been implemented Tenet-Performance Regression in measured performance of the product from goals.
Projects
None yet
Development

No branches or pull requests

5 participants