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.

Sign up for GitHub

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

Jump to bottom

Optimise allocations in AssertEx.NormalizeWhitespace and OperationTreeVerifier.Verify #75202

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
+359 −36
Open

Optimise allocations in AssertEx.NormalizeWhitespace and OperationTreeVerifier.Verify #75202

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
200a549
Initial commit
hamarb123 Sep 21, 2024
2c34a64
Address feedback (round 1)
hamarb123 Oct 2, 2024
5de2e77
Address `NormalizeWhitespaceROM` feedback
hamarb123 Oct 2, 2024
c330a69
Fix build error
hamarb123 Oct 2, 2024
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
138 changes: 120 additions & 18 deletions src/Compilers/Test/Core/Assert/AssertEx.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
#nullable disable

using System;
using System.Buffers;
using System.Collections;
using System.Collections.Generic;
using System.Collections.Immutable;
Expand Down Expand Up @@ -557,12 +558,33 @@ public static void AssertEqualToleratingWhitespaceDifferences(
[CallerFilePath] string expectedValueSourcePath = null,
[CallerLineNumber] int expectedValueSourceLine = 0)
{
var normalizedExpected = NormalizeWhitespace(expected);
var normalizedActual = NormalizeWhitespace(actual);
AssertEqualToleratingWhitespaceDifferences(expected.AsSpan(), actual.AsSpan(), message, escapeQuotes, expectedValueSourcePath, expectedValueSourceLine);
}

// compares against a baseline
public static void AssertEqualToleratingWhitespaceDifferences(
ReadOnlySpan<char> expected,
ReadOnlySpan<char> actual,
string message = null,
bool escapeQuotes = false,
[CallerFilePath] string expectedValueSourcePath = null,
[CallerLineNumber] int expectedValueSourceLine = 0)
{
var normalizedExpected = NormalizeWhitespace(expected, out var pooled1);
var normalizedActual = NormalizeWhitespace(actual, out var pooled2);

if (!normalizedExpected.SequenceEqual(normalizedActual))
{
Assert.True(false, GetAssertMessage(expected.ToString(), actual.ToString(), message, escapeQuotes, expectedValueSourcePath, expectedValueSourceLine));
}

if (normalizedExpected != normalizedActual)
if (pooled1 != null)
{
Assert.True(false, GetAssertMessage(expected, actual, message, escapeQuotes, expectedValueSourcePath, expectedValueSourceLine));
ArrayPool<char>.Shared.Return(pooled1);
}
if (pooled2 != null)
{
ArrayPool<char>.Shared.Return(pooled2);
}
}

Expand Down Expand Up @@ -594,37 +616,117 @@ public static void AssertResultsEqual(string result1, string result2)

public static void AssertContainsToleratingWhitespaceDifferences(string expectedSubString, string actualString)
{
expectedSubString = NormalizeWhitespace(expectedSubString);
actualString = NormalizeWhitespace(actualString);
Assert.Contains(expectedSubString, actualString, StringComparison.Ordinal);
AssertContainsToleratingWhitespaceDifferences(expectedSubString.AsSpan(), actualString.AsSpan());
}

public static void AssertContainsToleratingWhitespaceDifferences(ReadOnlySpan<char> expectedSubString, ReadOnlySpan<char> actualString)
{
expectedSubString = NormalizeWhitespace(expectedSubString, out var pooled1);
actualString = NormalizeWhitespace(actualString, out var pooled2);

if (!actualString.Contains(expectedSubString, StringComparison.Ordinal))
{
// Call through to actual API to get same error message experience
Assert.Contains(expectedSubString.ToString(), actualString.ToString(), StringComparison.Ordinal);
}

if (pooled1 != null)
{
ArrayPool<char>.Shared.Return(pooled1);
}
if (pooled2 != null)
{
ArrayPool<char>.Shared.Return(pooled2);
}
}

public static void AssertStartsWithToleratingWhitespaceDifferences(string expectedSubString, string actualString)
{
expectedSubString = NormalizeWhitespace(expectedSubString);
actualString = NormalizeWhitespace(actualString);
Assert.StartsWith(expectedSubString, actualString, StringComparison.Ordinal);
AssertStartsWithToleratingWhitespaceDifferences(expectedSubString.AsSpan(), actualString.AsSpan());
}

public static void AssertStartsWithToleratingWhitespaceDifferences(ReadOnlySpan<char> expectedSubString, ReadOnlySpan<char> actualString)
{
expectedSubString = NormalizeWhitespace(expectedSubString, out var pooled1);
actualString = NormalizeWhitespace(actualString, out var pooled2);

if (!actualString.StartsWith(expectedSubString, StringComparison.Ordinal))
{
// Call through to actual API to get same error message experience
Assert.StartsWith(expectedSubString.ToString(), actualString.ToString(), StringComparison.Ordinal);
}

if (pooled1 != null)
{
ArrayPool<char>.Shared.Return(pooled1);
}
hamarb123 marked this conversation as resolved.
Show resolved Hide resolved
if (pooled2 != null)
{
ArrayPool<char>.Shared.Return(pooled2);
}
}

internal static ReadOnlySpan<char> NormalizeWhitespace(ReadOnlySpan<char> input, out char[] pooledAlloc)
{
return NormalizeWhitespaceROM(input, out pooledAlloc).Span;
}

internal static string NormalizeWhitespace(string input)
internal static ReadOnlyMemory<char> NormalizeWhitespaceROM(ReadOnlySpan<char> input, out char[] pooledAlloc)
hamarb123 marked this conversation as resolved.
Show resolved Hide resolved
{
var output = new StringBuilder();
var inputLines = input.Split('\n', '\r');
foreach (var line in inputLines)
// Setup allocation (start with none)
pooledAlloc = null;
int allocSize = 0;

// Loop over each line section until we run out of lines
int prev = 0;
int next = input.IndexOfAny("\r\n".AsSpan());
if (next < 0) next = input.Length;
while (true)
Copy link
Member

Choose a reason for hiding this comment

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

Reading through this I'm wondering if it should be broken up a bit. Basically should we have one method that iterates through a ReadOnlySpan<char> and gives us back ReadOnlyMemory<char> for the lines within it?

Copy link
Author

Choose a reason for hiding this comment

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

I'm not exactly sure what you mean by this?

{
var trimmedLine = line.Trim();
// Get the line, trimmed
var trimmedLine = input.Slice(prev, next - prev).Trim();
if (trimmedLine.Length > 0)
{
// We want to append 2 spaces first if we don't start with { or }
int spacesToAppend = 0;
if (!(trimmedLine[0] == '{' || trimmedLine[0] == '}'))
{
output.Append(" ");
spacesToAppend = 2;
}

output.AppendLine(trimmedLine);
// Ensure buffer size is big enough
int newChars = spacesToAppend + trimmedLine.Length + Environment.NewLine.Length;
pooledAlloc ??= ArrayPool<char>.Shared.Rent(newChars);
if (pooledAlloc.Length < allocSize + newChars)
{
char[] newArr = ArrayPool<char>.Shared.Rent(allocSize + newChars);
pooledAlloc.AsSpan().Slice(0, allocSize).CopyTo(newArr.AsSpan().Slice(0, allocSize));
ArrayPool<char>.Shared.Return(pooledAlloc);
pooledAlloc = newArr;
Copy link
Member

Choose a reason for hiding this comment

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

This pattern of basically increasing the size of the pooled array is repeated a few times in the PR. Think we should factor this out into a helper method.

Copy link
Author

Choose a reason for hiding this comment

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

Do you mean another inline method, or in some helper type? If you mean in a helper type, which one, or where would I create it?

}

// Copy new text into buffer
pooledAlloc.AsSpan().Slice(allocSize, spacesToAppend).Fill(' ');
hamarb123 marked this conversation as resolved.
Show resolved Hide resolved
allocSize += spacesToAppend;
trimmedLine.CopyTo(pooledAlloc.AsSpan().Slice(allocSize, trimmedLine.Length));
allocSize += trimmedLine.Length;
Environment.NewLine.AsSpan().CopyTo(pooledAlloc.AsSpan().Slice(allocSize, Environment.NewLine.Length));
allocSize += Environment.NewLine.Length;
}

// Exit if done
if (next == input.Length) break;

// Find next section
prev = next + 1;
next = input.Slice(prev).IndexOfAny("\r\n".AsSpan());
if (next >= 0) next += prev;
else next = input.Length;
}

return output.ToString();
// Return the valid part of our allocation
if (pooledAlloc == null) return default;
hamarb123 marked this conversation as resolved.
Show resolved Hide resolved
return pooledAlloc.AsMemory().Slice(0, allocSize);
}

public static string GetAssertMessage(string expected, string actual, string prefix = null, bool escapeQuotes = false, string expectedValueSourcePath = null, int expectedValueSourceLine = 0)
Expand Down
2 changes: 1 addition & 1 deletion src/Compilers/Test/Core/Compilation/ControlFlowGraphVerifier.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1824,7 +1824,7 @@ public override void VisitFlowAnonymousFunction(IFlowAnonymousFunctionOperation
int id = _anonymousFunctionsMap.Count;
_anonymousFunctionsMap.Add(operation, g);
Assert.Equal(OperationKind.AnonymousFunction, g.OriginalOperation.Kind);
GetFlowGraph(_builder, _compilation, g, _region, $"#A{id}{_idSuffix}", _currentIndent.Length + 4, _associatedSymbol);
GetFlowGraph(_builder, _compilation, g, _region, $"#A{id}{_idSuffix}", _currentIndent + 4, _associatedSymbol);
LogString("}");
LogNewLine();
}
Expand Down
Loading