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

Reduce allocations in AbstractProjectExtensionProvider.FilterExtensions #74112

Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,19 @@ static ImmutableArray<CodeRefactoringProvider> GetProjectRefactorings(TextDocume
}

static ProjectCodeRefactoringProvider.ExtensionInfo GetExtensionInfo(ExportCodeRefactoringProviderAttribute attribute)
=> new(attribute.DocumentKinds, attribute.DocumentExtensions);
{
var kinds = new TextDocumentKind[attribute.DocumentKinds.Length];
for (var i = 0; i < kinds.Length; i++)
{
var kindString = attribute.DocumentKinds[i];
if (!Enum.TryParse(kindString, out TextDocumentKind kind))
kind = 0;

kinds[i] = kind;
}

return new(kinds, attribute.DocumentExtensions);
}
}

public async Task<bool> HasRefactoringsAsync(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
using System.Runtime.CompilerServices;
using Microsoft.CodeAnalysis.Diagnostics;
using Microsoft.CodeAnalysis.PooledObjects;
using Microsoft.CodeAnalysis.Shared.Extensions;
using Roslyn.Utilities;

namespace Microsoft.CodeAnalysis;
Expand All @@ -18,7 +19,7 @@ internal abstract class AbstractProjectExtensionProvider<TProvider, TExtension,
where TExportAttribute : Attribute
where TExtension : class
{
public record class ExtensionInfo(string[] DocumentKinds, string[]? DocumentExtensions);
public record class ExtensionInfo(TextDocumentKind[] DocumentKinds, string[]? DocumentExtensions);

// Following CWTs are used to cache completion providers from projects' references,
// so we can avoid the slow path unless there's any change to the references.
Expand Down Expand Up @@ -87,48 +88,52 @@ public static ImmutableArray<TExtension> GetExtensions(TextDocument document, Fu

public static ImmutableArray<TExtension> FilterExtensions(TextDocument document, ImmutableArray<TExtension> extensions, Func<TExportAttribute, ExtensionInfo> getExtensionInfoForFiltering)
{
return extensions.WhereAsArray(ShouldIncludeExtension);
return extensions.WhereAsArray(ShouldIncludeExtension, (document, getExtensionInfoForFiltering));

bool ShouldIncludeExtension(TExtension extension)
static bool ShouldIncludeExtension(TExtension extension, (TextDocument, Func<TExportAttribute, ExtensionInfo>) args)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i have no problem with you inlining this method into WhereAsArray.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tried it, stared at it for a minute, and I personally like it better in the current form. Will gladly change it though if you feel fairly strongly or have an objective reason.

{
(var document, var getExtensionInfoForFiltering) = args;
ToddGrun marked this conversation as resolved.
Show resolved Hide resolved
if (!s_extensionInfoMap.TryGetValue(extension, out var extensionInfo))
{
extensionInfo = s_extensionInfoMap.GetValue(extension,
new ConditionalWeakTable<TExtension, ExtensionInfo?>.CreateValueCallback(ComputeExtensionInfo));
}
extensionInfo = GetOrCreateExtensionInfo(extension, getExtensionInfoForFiltering);

if (extensionInfo == null)
return true;

if (!extensionInfo.DocumentKinds.Contains(document.Kind.ToString()))
if (-1 == Array.IndexOf(extensionInfo.DocumentKinds, document.Kind))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would prefer a < 0 check. even if doc'ed some apis end up returning different from -1.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!

return false;

if (document.FilePath != null &&
extensionInfo.DocumentExtensions != null &&
!extensionInfo.DocumentExtensions.Contains(PathUtilities.GetExtension(document.FilePath)))
-1 == Array.IndexOf(extensionInfo.DocumentExtensions, PathUtilities.GetExtension(document.FilePath)))
{
return false;
}

return true;
}

ExtensionInfo? ComputeExtensionInfo(TExtension extension)
static ExtensionInfo? GetOrCreateExtensionInfo(TExtension extension, Func<TExportAttribute, ExtensionInfo> getExtensionInfoForFiltering)
{
TExportAttribute? attribute;
try
{
var typeInfo = extension.GetType().GetTypeInfo();
attribute = typeInfo.GetCustomAttribute<TExportAttribute>();
}
catch
return s_extensionInfoMap.GetValue(extension,
new ConditionalWeakTable<TExtension, ExtensionInfo?>.CreateValueCallback(ComputeExtensionInfo));

ExtensionInfo? ComputeExtensionInfo(TExtension extension)
{
attribute = null;
}
TExportAttribute? attribute;
try
{
var typeInfo = extension.GetType().GetTypeInfo();
attribute = typeInfo.GetCustomAttribute<TExportAttribute>();
}
catch
{
attribute = null;
}

if (attribute == null)
return null;
return getExtensionInfoForFiltering(attribute);
if (attribute == null)
return null;
return getExtensionInfoForFiltering(attribute);
}
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -989,7 +989,19 @@ private ImmutableDictionary<DiagnosticId, ImmutableArray<CodeFixProvider>> Compu
}

private static ProjectCodeFixProvider.ExtensionInfo GetExtensionInfo(ExportCodeFixProviderAttribute attribute)
=> new(attribute.DocumentKinds, attribute.DocumentExtensions);
{
var kinds = new TextDocumentKind[attribute.DocumentKinds.Length];
for (var i = 0; i < kinds.Length; i++)
{
var kindString = attribute.DocumentKinds[i];
if (!Enum.TryParse(kindString, out TextDocumentKind kind))
kind = 0;

kinds[i] = kind;
}

return new(kinds, attribute.DocumentExtensions);
}
Copy link
Member

@CyrusNajmabadi CyrusNajmabadi Jun 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we not have a helper for this? (since it seems duplicated).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!


private sealed class FixerComparer : IComparer<CodeFixProvider>
{
Expand Down
Loading