Skip to content

Avoid per-call bool[] allocation in HeaderUtilities.AreEqualCollections #131230

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

Draft
mangod9 wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Avoid per-call bool[] allocation in HeaderUtilities.AreEqualCollections #131230

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
62 changes: 38 additions & 24 deletions src/libraries/System.Net.Http/src/System/Net/Http/Headers/HeaderUtilities.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -201,41 +201,55 @@ internal static bool AreEqualCollections<T>(ObjectCollection<T>? x, ObjectCollec

// We have two unordered lists. So comparison is an O(n*m) operation which is expensive. Usually
// headers have 1-2 parameters (if any), so this comparison shouldn't be too expensive.
bool[] alreadyFound = new bool[x.Count];

@EgorBo EgorBo Jul 22, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@AndyAyersMS this looks like a good candidate for escape analysis so all the unsafe code with stackalloc + arraypool becomes unnecessary

@AndyAyersMS AndyAyersMS Jul 22, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

#127980 will conditionally stack allocate this array. Its threshold is dynamic depending on how much free space exists on the stack. However the fallback is a heap allocation, not an array pool rental.

If we wanted to model this exact pattern we'd have to add quite a bit more logic to #127980 and some hellper-call accessible versions of rent/return.

@EgorBo EgorBo Jul 22, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I wonder if we can rewrite it to just bool[] alreadyFound = x.Count <= 32 ? new bool[32] : new bool[x.Count]; and it will just work we can assume the number (headers) is rather low.

@AndyAyersMS AndyAyersMS Jul 24, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yes, that stack allocates when the count is small.

The foreach below would need to change not to walk the entire array.

@AndyAyersMS AndyAyersMS Jul 24, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Also would still heap allocate in R2R. We never landed #116651 because the stack allocation pattern causes the method to drop out of R2R.

@mangod9 mangod9 Jul 24, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

so looks like it might still be worthwhile to make such changes in code?

int i = 0;
foreach (var xItem in x)
const int StackallocThreshold = 32;
int count = x.Count;
bool[]? rentedArray = count > StackallocThreshold ? ArrayPool<bool>.Shared.Rent(count) : null;
Span<bool> alreadyFound = rentedArray is null ? stackalloc bool[StackallocThreshold] : rentedArray;
alreadyFound = alreadyFound.Slice(0, count);
alreadyFound.Clear();
try
{
Debug.Assert(xItem != null);

i = 0;
bool found = false;
foreach (var yItem in y)
foreach (var xItem in x)
{
if (!alreadyFound[i])
Debug.Assert(xItem != null);

int i = 0;
bool found = false;
foreach (var yItem in y)
{
if (((comparer == null) && xItem.Equals(yItem)) ||
((comparer != null) && comparer.Equals(xItem, yItem)))
if (!alreadyFound[i])
{
alreadyFound[i] = true;
found = true;
break;
if (((comparer == null) && xItem.Equals(yItem)) ||
((comparer != null) && comparer.Equals(xItem, yItem)))
{
alreadyFound[i] = true;
found = true;
break;
}
}
i++;
}

if (!found)
{
return false;
}
i++;
}

if (!found)
// Since we never re-use a "found" value in 'y', we expect 'alreadyFound' to have all fields set to 'true'.
// Otherwise the two collections can't be equal and we should not get here.
Debug.Assert(!alreadyFound.Contains(false),
"Expected all values in 'alreadyFound' to be true since collections are considered equal.");

return true;
}
finally
{
if (rentedArray is not null)
{
return false;
ArrayPool<bool>.Shared.Return(rentedArray);
}
}

// Since we never re-use a "found" value in 'y', we expect 'alreadyFound' to have all fields set to 'true'.
// Otherwise the two collections can't be equal and we should not get here.
Debug.Assert(Array.TrueForAll(alreadyFound, value => value),
"Expected all values in 'alreadyFound' to be true since collections are considered equal.");

return true;
}

internal static int GetNextNonEmptyOrWhitespaceIndex(string input, int startIndex, bool skipEmptyValues,
Expand Down
Loading