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

Fix hot path string allocations from ProjectKey #10138

Merged
merged 2 commits into from
Mar 21, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -84,13 +84,7 @@ public static string GetNormalizedDirectoryName(string? filePath)
var filePathSpan = filePath.AsSpan();

using var _1 = ArrayPool<char>.Shared.GetPooledArray(filePathSpan.Length, out var array);
var normalizedSpan = NormalizeCoreAndGetSpan(filePathSpan, array);

var lastSlashIndex = normalizedSpan.LastIndexOf('/');

var directoryNameSpan = lastSlashIndex >= 0
? normalizedSpan[..(lastSlashIndex + 1)] // Include trailing slash
: normalizedSpan;
var directoryNameSpan = NormalizeDirectoryNameCore(filePathSpan, array);

if (filePathSpan.Equals(directoryNameSpan, StringComparison.Ordinal))
{
Expand All @@ -100,7 +94,30 @@ public static string GetNormalizedDirectoryName(string? filePath)
return CreateString(directoryNameSpan);
}

public static bool FilePathsEquivalent(string? filePath1, string? filePath2)
public static bool AreDirectoryPathsEquivalent(string? filePath1, string? filePath2)
{
var filePathSpan1 = filePath1.AsSpanOrDefault();
var filePathSpan2 = filePath2.AsSpanOrDefault();

if (filePathSpan1.IsEmpty)
{
return filePathSpan2.IsEmpty;
}
else if (filePathSpan2.IsEmpty)
{
return false;
}

using var _1 = ArrayPool<char>.Shared.GetPooledArray(filePathSpan1.Length, out var array1);
var normalizedSpan1 = NormalizeDirectoryNameCore(filePathSpan1, array1);

using var _2 = ArrayPool<char>.Shared.GetPooledArray(filePathSpan2.Length, out var array2);
var normalizedSpan2 = NormalizeDirectoryNameCore(filePathSpan2, array2);

return normalizedSpan1.Equals(normalizedSpan2, FilePathComparison.Instance);
}

public static bool AreFilePathsEquivalent(string? filePath1, string? filePath2)
{
var filePathSpan1 = filePath1.AsSpanOrDefault();
var filePathSpan2 = filePath2.AsSpanOrDefault();
Expand Down Expand Up @@ -151,6 +168,17 @@ private static ReadOnlySpan<char> NormalizeCoreAndGetSpan(ReadOnlySpan<char> sou
return destination.Slice(start, length);
}

private static ReadOnlySpan<char> NormalizeDirectoryNameCore(ReadOnlySpan<char> source, Span<char> destination)
{
var normalizedSpan = NormalizeCoreAndGetSpan(source, destination);

var lastSlashIndex = normalizedSpan.LastIndexOf('/');

return lastSlashIndex >= 0
? normalizedSpan[..(lastSlashIndex + 1)] // Include trailing slash
: normalizedSpan;
}

/// <summary>
/// Normalizes the given <paramref name="source"/> file path and writes the result in <paramref name="destination"/>.
/// </summary>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ private FilePathNormalizingComparer()
{
}

public bool Equals(string? x, string? y) => FilePathNormalizer.FilePathsEquivalent(x, y);
public bool Equals(string? x, string? y) => FilePathNormalizer.AreFilePathsEquivalent(x, y);

public int GetHashCode(string obj) => FilePathNormalizer.GetHashCode(obj);
}
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@

using System;
using System.Diagnostics;
using Microsoft.AspNetCore.Razor;
using Microsoft.AspNetCore.Razor.Utilities;

namespace Microsoft.CodeAnalysis.Razor.ProjectSystem;
Expand All @@ -19,7 +18,7 @@ namespace Microsoft.CodeAnalysis.Razor.ProjectSystem;
// end up. All creation logic is here in one place to ensure this is consistent.
public static ProjectKey From(HostProject hostProject) => new(hostProject.IntermediateOutputPath);
public static ProjectKey From(IProjectSnapshot project) => new(project.IntermediateOutputPath);
public static ProjectKey? From(Project project)
public static ProjectKey From(Project project)
DustinCampbell marked this conversation as resolved.
Show resolved Hide resolved
{
var intermediateOutputPath = FilePathNormalizer.GetNormalizedDirectoryName(project.CompilationOutputInfo.AssemblyPath);
return new(intermediateOutputPath);
Expand All @@ -33,8 +32,8 @@ private ProjectKey(string id)
{
Debug.Assert(id is not null, "Cannot create a key for null Id. Did you call ProjectKey.From(this) in a constructor, before initializing a property?");
Debug.Assert(!id!.EndsWith(".csproj", StringComparison.OrdinalIgnoreCase), "We expect the intermediate output path, not the project file");
// The null check in the assert means the compiler thinks we're lying about id being non-nullable.. which is fair I suppose.
Id = FilePathNormalizer.NormalizeDirectory(id).AssumeNotNull();

Id = FilePathNormalizer.NormalizeDirectory(id);
}

public override int GetHashCode()
Expand All @@ -46,4 +45,20 @@ public bool Equals(ProjectKey other)
{
return FilePathComparer.Instance.Equals(Id, other.Id);
}

/// <summary>
/// Returns <see langword="true"/> if this <see cref="ProjectKey"/> matches the given <see cref="Project"/>.
/// </summary>
public bool Matches(Project project)
{
// In order to perform this check, we are relying on the fact that Id will always end with a '/',
// because it is guaranteed to be normalized. However, CompilationOutputInfo.AssemblyPath will
// contain the assembly file name, which AreDirectoryPathsEquivalent will shave off before comparing.
// So, AreDirectoryPathsEquivalent will return true when Id is "C:/my/project/path/"
// and the assembly path is "C:\my\project\path\assembly.dll"

Debug.Assert(Id.EndsWith('/'), $"This method can't be called if {nameof(Id)} is not a normalized directory path.");

return FilePathNormalizer.AreDirectoryPathsEquivalent(Id, project.CompilationOutputInfo.AssemblyPath);
DustinCampbell marked this conversation as resolved.
Show resolved Hide resolved
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ public RemoteProjectSnapshot(Project project, DocumentSnapshotFactory documentSn
_project = project;
_documentSnapshotFactory = documentSnapshotFactory;
_telemetryReporter = telemetryReporter;
_projectKey = ProjectKey.From(_project).AssumeNotNull();
_projectKey = ProjectKey.From(_project);

_lazyConfiguration = new Lazy<RazorConfiguration>(CreateRazorConfiguration);
_lazyProjectEngine = new Lazy<RazorProjectEngine>(() =>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ private Document GetGeneratedDocument(VersionedDocumentContext documentContext)

// TODO: A real implementation needs to get the SourceGeneratedDocument from the solution

var projectKey = ProjectKey.From(razorDocument.Project).AssumeNotNull();
var projectKey = ProjectKey.From(razorDocument.Project);
var generatedFilePath = _filePathService.GetRazorCSharpFilePath(projectKey, razorDocument.FilePath.AssumeNotNull());
var generatedDocumentId = solution.GetDocumentIdsWithFilePath(generatedFilePath).First(d => d.ProjectId == razorDocument.Project.Id);
var generatedDocument = solution.GetDocument(generatedDocumentId).AssumeNotNull();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -354,7 +354,7 @@ private void ProjectManager_Changed(object? sender, ProjectChangeEventArgs args)

foreach (var project in workspace.CurrentSolution.Projects)
{
if (key.Equals(ProjectKey.From(project)))
if (key.Matches(project))
{
return project.Id;
}
Expand All @@ -363,20 +363,13 @@ private void ProjectManager_Changed(object? sender, ProjectChangeEventArgs args)
return null;
}

public ProjectKey? TryFindProjectKeyForProjectId(ProjectId projectId)
private ProjectKey? TryFindProjectKeyForProjectId(ProjectId projectId)
{
var workspace = _workspaceProvider.GetWorkspace();

var project = workspace.CurrentSolution.GetProject(projectId);
if (project is null ||
project.Language != LanguageNames.CSharp)
{
return null;
}

var projectKey = ProjectKey.From(project);

return projectKey;
return workspace.CurrentSolution.GetProject(projectId) is { Language: LanguageNames.CSharp } project
? ProjectKey.From(project)
: null;
}

private RazorDynamicFileInfo CreateEmptyInfo(Key key)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -166,10 +166,6 @@ private async Task RemoveFallbackDocumentAsync(ProjectId projectId, string fileP
}

var projectKey = ProjectKey.From(project);
if (projectKey is not { } razorProjectKey)
{
return;
}

var hostDocument = CreateHostDocument(filePath, projectFilePath);
if (hostDocument is null)
Expand All @@ -178,7 +174,7 @@ private async Task RemoveFallbackDocumentAsync(ProjectId projectId, string fileP
}

await UpdateProjectManagerAsync(
updater => updater.DocumentRemoved(razorProjectKey, hostDocument),
updater => updater.DocumentRemoved(projectKey, hostDocument),
cancellationToken)
.ConfigureAwait(false);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -496,12 +496,7 @@ private bool TryGetProjectSnapshot(Project? project, [NotNullWhen(true)] out IPr
return false;
}

// ProjectKey could be null, if Roslyn doesn't know the IntermediateOutputPath for the project
if (ProjectKey.From(project) is not { } projectKey)
{
projectSnapshot = null;
return false;
}
var projectKey = ProjectKey.From(project);

return _projectManager.TryGetLoadedProject(projectKey, out projectSnapshot);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ namespace Microsoft.AspNetCore.Razor.ProjectEngineHost.Test;

public class FilePathNormalizerTest(ITestOutputHelper testOutput) : ToolingTestBase(testOutput)
{
[OSSkipConditionFact(new[] { "OSX", "Linux" })]
[OSSkipConditionFact(["OSX", "Linux"])]
public void Normalize_Windows_StripsPrecedingSlash()
{
// Arrange
Expand Down Expand Up @@ -102,7 +102,7 @@ public void NormalizeDirectory_EndsWithoutSlash()
Assert.Equal("C:/path/to/directory/", normalized);
}

[OSSkipConditionFact(new[] { "OSX", "Linux" })]
[OSSkipConditionFact(["OSX", "Linux"])]
public void NormalizeDirectory_Windows_HandlesSingleSlashDirectory()
{
// Arrange
Expand All @@ -115,29 +115,49 @@ public void NormalizeDirectory_Windows_HandlesSingleSlashDirectory()
Assert.Equal("/", normalized);
}

[Theory]
[InlineData("path/to/", "path/to/", true)]
[InlineData("path/to1/", "path/to2/", false)]
[InlineData("path/to/", "path/to/file.cs", true)]
[InlineData("path/to/file.cs", "path/to/file.cs", true)]
[InlineData("path/to/file1.cs", "path/to/file2.cs", true)]
[InlineData("path/to1/file.cs", "path/to2/file.cs", false)]
[InlineData("path/to/", @"path\to\", true)]
[InlineData("path/to1/", @"path\to2\", false)]
[InlineData("path/to/", @"path\to\file.cs", true)]
[InlineData("path/to/file.cs", @"path\to\file.cs", true)]
[InlineData("path/to/file1.cs", @"path\to\file2.cs", true)]
[InlineData("path/to1/file.cs", @"path\to2\file.cs", false)]
public void AreDirectoryPathsEquivalent(string path1, string path2, bool expected)
{
var result = FilePathNormalizer.AreDirectoryPathsEquivalent(path1, path2);

Assert.Equal(expected, result);
}

[Fact]
public void FilePathsEquivalent_NotEqualPaths_ReturnsFalse()
public void AreFilePathsEquivalent_NotEqualPaths_ReturnsFalse()
{
// Arrange
var filePath1 = "path/to/document.cshtml";
var filePath2 = "path\\to\\different\\document.cshtml";

// Act
var result = FilePathNormalizer.FilePathsEquivalent(filePath1, filePath2);
var result = FilePathNormalizer.AreFilePathsEquivalent(filePath1, filePath2);

// Assert
Assert.False(result);
}

[Fact]
public void FilePathsEquivalent_NormalizesPathsBeforeComparison_ReturnsTrue()
public void AreFilePathsEquivalent_NormalizesPathsBeforeComparison_ReturnsTrue()
{
// Arrange
var filePath1 = "path/to/document.cshtml";
var filePath2 = "path\\to\\document.cshtml";

// Act
var result = FilePathNormalizer.FilePathsEquivalent(filePath1, filePath2);
var result = FilePathNormalizer.AreFilePathsEquivalent(filePath1, filePath2);

// Assert
Assert.True(result);
Expand Down Expand Up @@ -189,7 +209,7 @@ public void Normalize_EmptyFilePath_ReturnsEmptyString()
Assert.Equal("/", normalized);
}

[OSSkipConditionFact(new[] { "Windows" })]
[OSSkipConditionFact(["Windows"])]
public void Normalize_NonWindows_AddsLeadingForwardSlash()
{
// Arrange
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,6 @@ public static Uri GetUri(params string[] parts)

public static void AssertEquivalent(string? expectedFilePath, string? actualFilePath)
{
Assert.True(FilePathNormalizer.FilePathsEquivalent(expectedFilePath, actualFilePath));
Assert.True(FilePathNormalizer.AreFilePathsEquivalent(expectedFilePath, actualFilePath));
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -190,7 +190,7 @@ public async Task DynamicFileAdded_TrackedProject_AddsDocuments()
Assert.Equal(SomeProjectFile1.TargetPath, project.GetDocument(SomeProjectFile1.FilePath)!.TargetPath);
Assert.Equal(SomeProjectFile2.TargetPath, project.GetDocument(SomeProjectFile2.FilePath)!.TargetPath);
// The test data is created with a "\" so when the test runs on Linux, and direct string comparison wouldn't work
Assert.True(FilePathNormalizer.FilePathsEquivalent(SomeProjectNestedComponentFile3.TargetPath, project.GetDocument(SomeProjectNestedComponentFile3.FilePath)!.TargetPath));
Assert.True(FilePathNormalizer.AreFilePathsEquivalent(SomeProjectNestedComponentFile3.TargetPath, project.GetDocument(SomeProjectNestedComponentFile3.FilePath)!.TargetPath));
}

[UIFact]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -211,9 +211,9 @@ public async Task WorkspaceChanged_DocumentEvents_EnqueuesUpdatesForDependentPro

// Assert
Assert.Equal(3, _workQueueTestAccessor.Work.Count);
Assert.Contains(_workQueueTestAccessor.Work, u => u.Key == ProjectKey.From(_projectNumberOne).Value.Id);
Assert.Contains(_workQueueTestAccessor.Work, u => u.Key == ProjectKey.From(_projectNumberTwo).Value.Id);
Assert.Contains(_workQueueTestAccessor.Work, u => u.Key == ProjectKey.From(_projectNumberThree).Value.Id);
Assert.Contains(_workQueueTestAccessor.Work, u => u.Key == ProjectKey.From(_projectNumberOne).Id);
Assert.Contains(_workQueueTestAccessor.Work, u => u.Key == ProjectKey.From(_projectNumberTwo).Id);
Assert.Contains(_workQueueTestAccessor.Work, u => u.Key == ProjectKey.From(_projectNumberThree).Id);

_workQueueTestAccessor.BlockBackgroundWorkStart.Set();
_workQueueTestAccessor.NotifyBackgroundWorkCompleted.Wait();
Expand Down Expand Up @@ -261,9 +261,9 @@ public async Task WorkspaceChanged_ProjectEvents_EnqueuesUpdatesForDependentProj

// Assert
Assert.Equal(3, _workQueueTestAccessor.Work.Count);
Assert.Contains(_workQueueTestAccessor.Work, u => u.Key == ProjectKey.From(_projectNumberOne).Value.Id);
Assert.Contains(_workQueueTestAccessor.Work, u => u.Key == ProjectKey.From(_projectNumberTwo).Value.Id);
Assert.Contains(_workQueueTestAccessor.Work, u => u.Key == ProjectKey.From(_projectNumberThree).Value.Id);
Assert.Contains(_workQueueTestAccessor.Work, u => u.Key == ProjectKey.From(_projectNumberOne).Id);
Assert.Contains(_workQueueTestAccessor.Work, u => u.Key == ProjectKey.From(_projectNumberTwo).Id);
Assert.Contains(_workQueueTestAccessor.Work, u => u.Key == ProjectKey.From(_projectNumberThree).Id);

_workQueueTestAccessor.BlockBackgroundWorkStart.Set();
_workQueueTestAccessor.NotifyBackgroundWorkCompleted.Wait();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,4 +22,8 @@ public static ReadOnlyMemory<char> AsMemoryOrDefault(this string? s)
// This method doesn't exist on .NET Framework, but it does on .NET Core.
public static bool Contains(this string s, char ch)
=> s.IndexOf(ch) >= 0;

// This method doesn't exist on .NET Framework, but it does on .NET Core.
public static bool EndsWith(this string s, char ch)
=> s.Length > 0 && s[^1] == ch;
}