Skip to content

Commit

Permalink
[Performance] Reduce array allocations of BadArguments (#69970)
Browse files Browse the repository at this point in the history 
* [Performance] Reduce array allocations of BadArguments

* Apply suggestions from code review

* Use `TrueBits().First()` in `FirstBadArgument` implementation

* Apply suggestion from code review

* Apply suggestions from code review

* Address review comment
  • Loading branch information
@Youssef1313
Youssef1313 authored Sep 20, 2023
1 parent 1e1f73d commit 57ebb12
Show file tree
Hide file tree
Showing 3 changed files with 61 additions and 33 deletions.
41 changes: 30 additions & 11 deletions src/Compilers/CSharp/Portable/Binder/Semantics/OverloadResolution/MemberAnalysisResult.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
using System.Collections.Immutable;
using System.Diagnostics;
using System.Diagnostics.CodeAnalysis;
using System.Linq;
using Microsoft.CodeAnalysis.CSharp.Symbols;
using Roslyn.Utilities;

Expand All @@ -17,7 +18,16 @@ internal readonly struct MemberAnalysisResult
{
// put these first for better packing
public readonly ImmutableArray<Conversion> ConversionsOpt;
public readonly ImmutableArray<int> BadArgumentsOpt;

/// <summary>
/// A bit vector representing whose true bits indicate indices of bad arguments
/// </summary>
/// <remarks>
/// The capacity of this BitVector might not match the parameter count of the method overload being resolved.
/// For example, if a method overload has 5 parameters and the second parameter is the only bad parameter, then this
/// BitVector could end up with Capacity being 2 where BadArguments[0] is false and BadArguments[1] is true.
/// </remarks>
public readonly BitVector BadArgumentsOpt;
public readonly ImmutableArray<int> ArgsToParamsOpt;
public readonly ImmutableArray<TypeParameterDiagnosticInfo> ConstraintFailureDiagnostics;

Expand All @@ -32,7 +42,7 @@ internal readonly struct MemberAnalysisResult

private MemberAnalysisResult(
MemberResolutionKind kind,
ImmutableArray<int> badArgumentsOpt = default,
BitVector badArgumentsOpt = default,
ImmutableArray<int> argsToParamsOpt = default,
ImmutableArray<Conversion> conversionsOpt = default,
int missingParameter = -1,
Expand Down Expand Up @@ -79,6 +89,8 @@ public int ParameterFromArgument(int arg)
return ArgsToParamsOpt[arg];
}

public int FirstBadArgument => BadArgumentsOpt.TrueBits().First();

// A method may be applicable, but worse than another method.
public bool IsApplicable
{
Expand Down Expand Up @@ -170,35 +182,42 @@ public static MemberAnalysisResult NameUsedForPositional(int argumentPosition)
{
return new MemberAnalysisResult(
MemberResolutionKind.NameUsedForPositional,
badArgumentsOpt: ImmutableArray.Create<int>(argumentPosition));
badArgumentsOpt: CreateBadArgumentsWithPosition(argumentPosition));
}

public static MemberAnalysisResult BadNonTrailingNamedArgument(int argumentPosition)
{
return new MemberAnalysisResult(
MemberResolutionKind.BadNonTrailingNamedArgument,
badArgumentsOpt: ImmutableArray.Create<int>(argumentPosition));
badArgumentsOpt: CreateBadArgumentsWithPosition(argumentPosition));
}

public static MemberAnalysisResult NoCorrespondingParameter(int argumentPosition)
{
return new MemberAnalysisResult(
MemberResolutionKind.NoCorrespondingParameter,
badArgumentsOpt: ImmutableArray.Create<int>(argumentPosition));
badArgumentsOpt: CreateBadArgumentsWithPosition(argumentPosition));
}

public static MemberAnalysisResult NoCorrespondingNamedParameter(int argumentPosition)
{
return new MemberAnalysisResult(
MemberResolutionKind.NoCorrespondingNamedParameter,
badArgumentsOpt: ImmutableArray.Create<int>(argumentPosition));
badArgumentsOpt: CreateBadArgumentsWithPosition(argumentPosition));
}

public static MemberAnalysisResult DuplicateNamedArgument(int argumentPosition)
{
return new MemberAnalysisResult(
MemberResolutionKind.DuplicateNamedArgument,
badArgumentsOpt: ImmutableArray.Create<int>(argumentPosition));
badArgumentsOpt: CreateBadArgumentsWithPosition(argumentPosition));
}

internal static BitVector CreateBadArgumentsWithPosition(int argumentPosition)
{
var badArguments = BitVector.Create(argumentPosition + 1);
badArguments[argumentPosition] = true;
return badArguments;
}

public static MemberAnalysisResult RequiredParameterMissing(int parameterPosition)
Expand All @@ -218,10 +237,10 @@ public static MemberAnalysisResult UnsupportedMetadata()
return new MemberAnalysisResult(MemberResolutionKind.UnsupportedMetadata);
}

public static MemberAnalysisResult BadArgumentConversions(ImmutableArray<int> argsToParamsOpt, ImmutableArray<int> badArguments, ImmutableArray<Conversion> conversions)
public static MemberAnalysisResult BadArgumentConversions(ImmutableArray<int> argsToParamsOpt, BitVector badArguments, ImmutableArray<Conversion> conversions)
{
Debug.Assert(conversions.Length != 0);
Debug.Assert(badArguments.Length != 0);
Debug.Assert(badArguments.TrueBits().Any());
return new MemberAnalysisResult(
MemberResolutionKind.BadArgumentConversion,
badArguments,
Expand Down Expand Up @@ -273,12 +292,12 @@ public static MemberAnalysisResult LessDerived()

public static MemberAnalysisResult NormalForm(ImmutableArray<int> argsToParamsOpt, ImmutableArray<Conversion> conversions, bool hasAnyRefOmittedArgument)
{
return new MemberAnalysisResult(MemberResolutionKind.ApplicableInNormalForm, default(ImmutableArray<int>), argsToParamsOpt, conversions, hasAnyRefOmittedArgument: hasAnyRefOmittedArgument);
return new MemberAnalysisResult(MemberResolutionKind.ApplicableInNormalForm, BitVector.Null, argsToParamsOpt, conversions, hasAnyRefOmittedArgument: hasAnyRefOmittedArgument);
}

public static MemberAnalysisResult ExpandedForm(ImmutableArray<int> argsToParamsOpt, ImmutableArray<Conversion> conversions, bool hasAnyRefOmittedArgument)
{
return new MemberAnalysisResult(MemberResolutionKind.ApplicableInExpandedForm, default(ImmutableArray<int>), argsToParamsOpt, conversions, hasAnyRefOmittedArgument: hasAnyRefOmittedArgument);
return new MemberAnalysisResult(MemberResolutionKind.ApplicableInExpandedForm, BitVector.Null, argsToParamsOpt, conversions, hasAnyRefOmittedArgument: hasAnyRefOmittedArgument);
}

public static MemberAnalysisResult Worse()
Expand Down
28 changes: 18 additions & 10 deletions src/Compilers/CSharp/Portable/Binder/Semantics/OverloadResolution/OverloadResolution.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3729,7 +3729,7 @@ private MemberAnalysisResult IsApplicable(
// * for a ref or out parameter, the type of the argument is identical to the type of the corresponding
// parameter. After all, a ref or out parameter is an alias for the argument passed.
ArrayBuilder<Conversion> conversions = null;
ArrayBuilder<int> badArguments = null;
BitVector badArguments = default;
for (int argumentPosition = 0; argumentPosition < paramCount; argumentPosition++)
{
BoundExpression argument = arguments.Argument(argumentPosition);
Expand All @@ -3744,8 +3744,12 @@ private MemberAnalysisResult IsApplicable(
}
else
{
badArguments = badArguments ?? ArrayBuilder<int>.GetInstance();
badArguments.Add(argumentPosition);
if (badArguments.IsNull)
{
badArguments = BitVector.Create(argumentPosition + 1);
}

badArguments[argumentPosition] = true;
conversion = Conversion.NoConversion;
}
}
Expand Down Expand Up @@ -3798,15 +3802,19 @@ private MemberAnalysisResult IsApplicable(
// lambda binding in particular, for instance, with LINQ expressions.
// Note that BuildArgumentsForErrorRecovery will still bind some number
// of overloads for the semantic model.
Debug.Assert(badArguments == null);
Debug.Assert(badArguments.IsNull);
Debug.Assert(conversions == null);
return MemberAnalysisResult.BadArgumentConversions(argsToParameters, ImmutableArray.Create(argumentPosition), ImmutableArray.Create(conversion));
return MemberAnalysisResult.BadArgumentConversions(argsToParameters, MemberAnalysisResult.CreateBadArgumentsWithPosition(argumentPosition), ImmutableArray.Create(conversion));
}

if (!conversion.Exists)
{
badArguments ??= ArrayBuilder<int>.GetInstance();
badArguments.Add(argumentPosition);
if (badArguments.IsNull)
{
badArguments = BitVector.Create(argumentPosition + 1);
}

badArguments[argumentPosition] = true;
}
}

Expand All @@ -3821,17 +3829,17 @@ private MemberAnalysisResult IsApplicable(
conversions.Add(conversion);
}

if (badArguments != null && !completeResults)
if (!badArguments.IsNull && !completeResults)
{
break;
}
}

MemberAnalysisResult result;
var conversionsArray = conversions != null ? conversions.ToImmutableAndFree() : default(ImmutableArray<Conversion>);
if (badArguments != null)
if (!badArguments.IsNull)
{
result = MemberAnalysisResult.BadArgumentConversions(argsToParameters, badArguments.ToImmutableAndFree(), conversionsArray);
result = MemberAnalysisResult.BadArgumentConversions(argsToParameters, badArguments, conversionsArray);
}
else
{
Expand Down
25 changes: 13 additions & 12 deletions ...Compilers/CSharp/Portable/Binder/Semantics/OverloadResolution/OverloadResolutionResult.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@

using System.Collections.Immutable;
using System.Diagnostics;
using System.Linq;
using Microsoft.CodeAnalysis.CSharp.Symbols;
using Microsoft.CodeAnalysis.PooledObjects;
using Roslyn.Utilities;
Expand Down Expand Up @@ -395,7 +396,7 @@ internal void ReportDiagnostics<T>(
break;
case MemberResolutionKind.NoCorrespondingNamedParameter:
if (supportedInPriorityOrder[noCorrespondingNamedParameterPriority].IsNull ||
result.Result.BadArgumentsOpt[0] > supportedInPriorityOrder[noCorrespondingNamedParameterPriority].Result.BadArgumentsOpt[0])
result.Result.FirstBadArgument > supportedInPriorityOrder[noCorrespondingNamedParameterPriority].Result.FirstBadArgument)
{
supportedInPriorityOrder[noCorrespondingNamedParameterPriority] = result;
}
Expand All @@ -419,22 +420,22 @@ internal void ReportDiagnostics<T>(
break;
case MemberResolutionKind.NameUsedForPositional:
if (supportedInPriorityOrder[nameUsedForPositionalPriority].IsNull ||
result.Result.BadArgumentsOpt[0] > supportedInPriorityOrder[nameUsedForPositionalPriority].Result.BadArgumentsOpt[0])
result.Result.FirstBadArgument > supportedInPriorityOrder[nameUsedForPositionalPriority].Result.FirstBadArgument)
{
supportedInPriorityOrder[nameUsedForPositionalPriority] = result;
}
break;
case MemberResolutionKind.BadNonTrailingNamedArgument:
if (supportedInPriorityOrder[badNonTrailingNamedArgumentPriority].IsNull ||
result.Result.BadArgumentsOpt[0] > supportedInPriorityOrder[badNonTrailingNamedArgumentPriority].Result.BadArgumentsOpt[0])
result.Result.FirstBadArgument > supportedInPriorityOrder[badNonTrailingNamedArgumentPriority].Result.FirstBadArgument)
{
supportedInPriorityOrder[badNonTrailingNamedArgumentPriority] = result;
}
break;
case MemberResolutionKind.DuplicateNamedArgument:
{
if (supportedInPriorityOrder[duplicateNamedArgumentPriority].IsNull ||
result.Result.BadArgumentsOpt[0] > supportedInPriorityOrder[duplicateNamedArgumentPriority].Result.BadArgumentsOpt[0])
result.Result.FirstBadArgument > supportedInPriorityOrder[duplicateNamedArgumentPriority].Result.FirstBadArgument)
{
supportedInPriorityOrder[duplicateNamedArgumentPriority] = result;
}
Expand Down Expand Up @@ -471,7 +472,7 @@ internal void ReportDiagnostics<T>(
if (firstSupported.Member is FunctionPointerMethodSymbol
&& firstSupported.Result.Kind == MemberResolutionKind.NoCorrespondingNamedParameter)
{
int badArg = firstSupported.Result.BadArgumentsOpt[0];
int badArg = firstSupported.Result.FirstBadArgument;
Debug.Assert(arguments.Names[badArg].HasValue);
Location badName = arguments.Names[badArg].GetValueOrDefault().Location;
diagnostics.Add(ErrorCode.ERR_FunctionPointersCannotBeCalledWithNamedArguments, badName);
Expand Down Expand Up @@ -770,7 +771,7 @@ private static void ReportNameUsedForPositional(
AnalyzedArguments arguments,
ImmutableArray<Symbol> symbols)
{
int badArg = bad.Result.BadArgumentsOpt[0];
int badArg = bad.Result.FirstBadArgument;
// We would not have gotten this error had there not been a named argument.
Debug.Assert(arguments.Names.Count > badArg);
Debug.Assert(arguments.Names[badArg].HasValue);
Expand All @@ -790,7 +791,7 @@ private static void ReportBadNonTrailingNamedArgument(
AnalyzedArguments arguments,
ImmutableArray<Symbol> symbols)
{
int badArg = bad.Result.BadArgumentsOpt[0];
int badArg = bad.Result.FirstBadArgument;
// We would not have gotten this error had there not been a named argument.
Debug.Assert(arguments.Names.Count > badArg);
Debug.Assert(arguments.Names[badArg].HasValue);
Expand All @@ -806,9 +807,9 @@ private static void ReportBadNonTrailingNamedArgument(

private static void ReportDuplicateNamedArgument(MemberResolutionResult<TMember> result, BindingDiagnosticBag diagnostics, AnalyzedArguments arguments)
{
Debug.Assert(result.Result.BadArgumentsOpt.Length == 1);
Debug.Assert(arguments.Names[result.Result.BadArgumentsOpt[0]].HasValue);
(string name, Location location) = arguments.Names[result.Result.BadArgumentsOpt[0]].GetValueOrDefault();
Debug.Assert(result.Result.BadArgumentsOpt.TrueBits().Count() == 1);
Debug.Assert(arguments.Names[result.Result.FirstBadArgument].HasValue);
(string name, Location location) = arguments.Names[result.Result.FirstBadArgument].GetValueOrDefault();
Debug.Assert(name != null);

// CS: Named argument '{0}' cannot be specified multiple times
Expand All @@ -830,7 +831,7 @@ private static void ReportNoCorrespondingNamedParameter(
// call was inapplicable because there was a bad name, that's a candidate
// for the "best" overload.

int badArg = bad.Result.BadArgumentsOpt[0];
int badArg = bad.Result.FirstBadArgument;
// We would not have gotten this error had there not been a named argument.
Debug.Assert(arguments.Names.Count > badArg);
Debug.Assert(arguments.Names[badArg].HasValue);
Expand Down Expand Up @@ -1105,7 +1106,7 @@ private bool HadBadArguments(
diagnostics.Add(ErrorCode.ERR_BadArgTypesForCollectionAdd, location, symbols, method);
}

foreach (var arg in badArg.Result.BadArgumentsOpt)
foreach (var arg in badArg.Result.BadArgumentsOpt.TrueBits())
{
ReportBadArgumentError(diagnostics, binder, name, arguments, symbols, badArg, method, arg);
}
Expand Down

0 comments on commit 57ebb12

Please sign in to comment.