Skip to content

Commit

Permalink
Further optimize FindValidCloneMethod
Browse files Browse the repository at this point in the history
#48935 optimizes one path that hits FindValidCloneMethod, but there are others that could do with the same fast-path optimization. In order to accomplish that, I introduced a new HasPossibleCloneMethod on NamedTypeSymbol, that returns whether the type _could_ have a method on it that is a valid clone method. For source types, this is a quick syntax check to see if it's a record. For metadata types, it's a check on just the member names for something with the right name. This should prevent us from unnecessarily loading all method signatures from metadata unnecessarily whenever we check for a clone method.
  • Loading branch information
333fred committed Oct 27, 2020
1 parent 7a8df88 commit a3ada0c
Show file tree
Hide file tree
Showing 10 changed files with 36 additions and 11 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -347,6 +347,8 @@ public override int GetHashCode()
{
return this.TypeDescriptor.GetHashCode();
}

internal override bool HasPossibleWellKnownCloneMethod() => false;
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -507,6 +507,8 @@ private SynthesizedAttributeData TrySynthesizeDebuggerDisplayAttribute()
WellKnownMember.System_Diagnostics_DebuggerDisplayAttribute__Type,
new TypedConstant(Manager.System_String, TypedConstantKind.Primitive, "<Anonymous Type>"))));
}

internal override bool HasPossibleWellKnownCloneMethod() => false;
}
}
}
1 change: 1 addition & 0 deletions src/Compilers/CSharp/Portable/Symbols/ErrorTypeSymbol.cs
Original file line number Diff line number Diff line change
Expand Up @@ -544,6 +544,7 @@ protected sealed override ITypeSymbol CreateITypeSymbol(CodeAnalysis.NullableAnn
}

internal override bool IsRecord => false;
internal override bool HasPossibleWellKnownCloneMethod() => false;
}

internal abstract class SubstitutedErrorTypeSymbol : ErrorTypeSymbol
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -560,16 +560,6 @@ internal override bool IsRecord
{
get
{
// First, do a check to see if there are even any members named <Clone>$. If we have to actually find the clone
// method, it can be expensive, because we have to load all the members. MemberNames just loads the strings, which
// is much cheaper.
if (!MemberNames.Contains(WellKnownMemberNames.CloneMethodName))
{
return false;
}

// There exists a member named <Clone>$. We still need to check to see if it's actually a valid clone method, but
// there's no getting around that now.
HashSet<DiagnosticInfo> useSiteDiagnostics = null;
return SynthesizedRecordClone.FindValidCloneMethod(this, ref useSiteDiagnostics) != null;
}
Expand Down Expand Up @@ -1429,6 +1419,9 @@ public override ImmutableArray<Symbol> GetMembers(string name)
return m;
}

internal override bool HasPossibleWellKnownCloneMethod()
=> MemberNames.Contains(WellKnownMemberNames.CloneMethodName);

internal override FieldSymbol FixedElementField
{
get
Expand Down
7 changes: 7 additions & 0 deletions src/Compilers/CSharp/Portable/Symbols/NamedTypeSymbol.cs
Original file line number Diff line number Diff line change
Expand Up @@ -511,6 +511,13 @@ internal abstract bool MangleName
/// no members with this name, returns an empty ImmutableArray. Never returns null.</returns>
public abstract override ImmutableArray<Symbol> GetMembers(string name);

/// <summary>
/// A lightweight check for whether this type has a possible clone method. This is less costly than GetMembers,
/// particularly for PE symbols, and can be used as a cheap heuristic for whether to fully search through all
/// members of this type for a valid clone method.
/// </summary>
internal abstract bool HasPossibleWellKnownCloneMethod();

internal virtual ImmutableArray<Symbol> GetSimpleNonTypeMembers(string name)
{
return GetMembers(name);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1249,6 +1249,13 @@ public sealed override ImmutableArray<Symbol> GetMembers(string name)
return ImmutableArray<Symbol>.Empty;
}


internal override bool HasPossibleWellKnownCloneMethod()
// For source symbols, there can only be a valid clone method if this is a record, which is a
// simple syntax check. This will need to change when we generalize cloning, but it's a good
// heuristic for now.
=> IsRecord;

internal override ImmutableArray<Symbol> GetSimpleNonTypeMembers(string name)
{
if (_lazyMembersDictionary != null || declaration.MemberNames.Contains(name))
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -144,7 +144,15 @@ internal override void GenerateMethodBody(TypeCompilationState compilationState,

internal static MethodSymbol? FindValidCloneMethod(TypeSymbol containingType, ref HashSet<DiagnosticInfo>? useSiteDiagnostics)
{
if (containingType.IsObjectType())
if (containingType.IsObjectType() || containingType is not NamedTypeSymbol containingNamedType)
{
return null;
}

// If this symbol is from metadata, getting all members can cause us to realize a lot of structures that we otherwise
// don't have to. Optimize for the common case here of there not being a method named <Clone>$. If there is a method
// with that name, it's most likely the one we're interested in, and we can't get around loading everything to find it.
if (!containingNamedType.HasPossibleWellKnownCloneMethod())
{
return null;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -221,5 +221,7 @@ internal override IEnumerable<FieldSymbol> GetFieldsToEmit()
internal sealed override NamedTypeSymbol AsNativeInteger() => throw ExceptionUtilities.Unreachable;

internal sealed override NamedTypeSymbol NativeIntegerUnderlyingType => null;

internal override bool HasPossibleWellKnownCloneMethod() => false;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -179,6 +179,7 @@ internal override void AddSynthesizedAttributes(PEModuleBuilder moduleBuilder, r
internal sealed override NamedTypeSymbol AsNativeInteger() => throw ExceptionUtilities.Unreachable;

internal sealed override NamedTypeSymbol NativeIntegerUnderlyingType => null;
internal override bool HasPossibleWellKnownCloneMethod() => false;
}

/// <summary>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -256,5 +256,7 @@ internal override bool GetGuidString(out string guidString)
{
return _underlyingType.GetGuidString(out guidString);
}

internal override bool HasPossibleWellKnownCloneMethod() => _underlyingType.HasPossibleWellKnownCloneMethod();
}
}

0 comments on commit a3ada0c

Please sign in to comment.