Skip to content

Commit

Permalink
Fix hot path string allocations from ProjectKey (#10138)
Browse files Browse the repository at this point in the history
@ToddGrun pointed me to a fair amount of string allocation due
`ProjectKey.From(Project)` being called repeatedly on a hot path. To
address this, I've added `ProjectKey.Matches(Project)`, which simply
compares the current key's `Id` with the project's intermediate output
path. This uses a new
`FilePathNormalizer.AreDirectoryPathsEquivalent(...)` helper to avoid
allocating a new string.


![image](https://github.com/dotnet/razor/assets/116161/0efd9ffd-0405-4b03-b2d1-f8b2ac9d0c6e)
  • Loading branch information
DustinCampbell committed Mar 21, 2024
2 parents f7afe2c + cec11f4 commit 08189eb
Show file tree
Hide file tree
Showing 13 changed files with 104 additions and 53 deletions.
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
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);
}
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)
{
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);
}
}
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
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
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
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
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
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
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));
}
}
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
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
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;
}

0 comments on commit 08189eb

Please sign in to comment.