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

[Http] Remove some unsafe code and save a string allocation #31267

Merged
merged 6 commits into from
Mar 30, 2021
Merged

[Http] Remove some unsafe code and save a string allocation #31267

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
08edaf0
[Http] Remove some unsafe code and save a string allocation
BrennanConroy Mar 26, 2021
6d984ce
fb
BrennanConroy Mar 26, 2021
2455646
fb
BrennanConroy Mar 26, 2021
a03d204
fb
BrennanConroy Mar 26, 2021
58e6752
stackalloc
BrennanConroy Mar 26, 2021
a6e5563
Get exact base64 length, and remove another allocation
BrennanConroy Mar 29, 2021
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
16 changes: 4 additions & 12 deletions src/Http/Headers/src/ContentDispositionHeaderValue.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -532,19 +532,11 @@ private bool RequiresEncoding(StringSegment input)
}

// Encode using MIME encoding
private unsafe string EncodeMime(StringSegment input)
private string EncodeMime(StringSegment input)
{
fixed (char* chars = input.Buffer)
{
var byteCount = Encoding.UTF8.GetByteCount(chars + input.Offset, input.Length);
var buffer = new byte[byteCount];
fixed (byte* bytes = buffer)
{
Encoding.UTF8.GetBytes(chars + input.Offset, input.Length, bytes, byteCount);
}
var encodedName = Convert.ToBase64String(buffer);
return "=?utf-8?B?" + encodedName + "?=";
}
var buffer = Encoding.UTF8.GetBytes(new ReadOnlySequence<char>(input.AsMemory()));
Copy link
Member

Choose a reason for hiding this comment

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

Don't create a ReadOnlySequence, that's the wrong overload 😄

Copy link
Member Author

@BrennanConroy BrennanConroy Mar 26, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

There is no Span<char> or ReadOnlySpan<char> overload that I could see.
The closest is int GetBytes(ReadOnlySpan<char> chars, Span<byte> bytes);

Copy link
Member

@davidfowl davidfowl Mar 26, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I see, yes, we don't want to allocate a buffer here 😄 . This is the missing overload of Convert.ToBase64String that we need! Conversion from utf16 to utf16:

public class Convert
{
+    public static int ToBase64String(Span<char> input, Span<char> output);
+    public static string ToBase64String(Span<char> input);
}

@GrabYourPitchforks thoughts? I'd like to get an API added for this on Convert wdyt?

Copy link
Member

Choose a reason for hiding this comment

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

@BrennanConroy go ahead with what you have for now. I'll file an issue on dotnet/runtime.

Copy link
Member

@JamesNK JamesNK Mar 26, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

What about using https://docs.microsoft.com/en-us/dotnet/api/system.buffers.text.base64?

You'd need to encode string into UTF8 bytes using stackalloc byte[] first. You could then encode in place (stackalloc would need to be long enough to go from bytes to base64), set consts prefix and suffix bytes (again, stackalloc byte[] would need to be long enough), and then turn the byte[] back into a string.

You can reduce this method down to one string allocation. I don't know how often this is called so probably not worth the complexity.

Copy link
Member

Choose a reason for hiding this comment

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

https://github.com/dotnet/runtime/blob/82ca681cbac89d813a3ce397e0c665e6c051ed67/src/libraries/System.Memory/src/System/Buffers/Text/Base64Encoder.cs#L155 that's the same and simpler calculation. Or use that method, it's already public.

You don't need an OperationStatus-based API or any pre-buffer or post-buffer for this proposal.

Right. But this needs a new Convert.ToBase64Span method.
My concern is only about having two base64-API sets. One in Convert-class and one in System.Buffers.Text.Base64. I'd prefer the latter, as it's name clearly indicates it is about base64.

Base64.EncodeToUtf8InPlace

@BrennanConroy just to note this method is not vectorized, whilst Base64.EncodeToUtf8 is for size >= 16. So depending on the measure here it may be more advantageous (see second snippet above).

Copy link
Member Author

Choose a reason for hiding this comment

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

For some reason the following code is slower and supposedly allocates more than the current impl (using BenchmarkDotnet):

private static ReadOnlySpan<char> MimePrefix => new char[] { '"', '=', '?', 'u', 't', 'f', '-', '8', '?', 'B', '?' };
private static ReadOnlySpan<char> MimeSuffix => new char[] { '?', '=', '"' };
// ...

var utf8Length = Encoding.UTF8.GetByteCount(input.AsSpan());
var buffer = ArrayPool<byte>.Shared.Rent(utf8Length);
try
{
	Encoding.UTF8.GetBytes(input.AsSpan(), buffer);

	return string.Create(MimePrefix.Length + MimeSuffix.Length + GetBase64Length(utf8Length), (buffer, utf8Length), static (span, state) =>
	{
		MimePrefix.CopyTo(span);
		span = span.Slice(MimePrefix.Length);
		var ret = Convert.TryToBase64Chars(state.buffer.AsSpan().Slice(0, state.utf8Length),
			span, out var written);
		Debug.Assert(ret);

		span = span.Slice(written);
		MimeSuffix.CopyTo(span);

		if (span.Length != MimeSuffix.Length)
		{
			throw new InvalidOperationException();
		}
	});
}
finally
{
	ArrayPool<byte>.Shared.Return(buffer);
}

And test:

var contentDisposition = new ContentDispositionHeaderValue("inline");
contentDisposition.FileName = "FileÃName.bat";

Comment above:

Method Mean Error StdDev Op/s Gen 0 Gen 1 Gen 2 Allocated
CD 977.1 ns 32.89 ns 95.43 ns 1,023,416.6 0.0156 - - 552 B

Current:

Method Mean Error StdDev Op/s Gen 0 Gen 1 Gen 2 Allocated
CD 790.1 ns 16.70 ns 46.56 ns 1,265,685.7 0.0078 - - 312 B

I'm going to stop work on this and merge as is assuming there are no glaring issues with the recent changes during PR review.

Copy link
Member

Choose a reason for hiding this comment

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

For some reason the following code is slower and supposedly allocates more than the current impl (using BenchmarkDotnet):

The Roslyn optimization that embeds arrays of consts exposed as ReadOnlySpans into the data section, thereby avoiding a per-access array allocation, only applies when the array is a byte[], sbyte[], or bool[], i.e. when it's an array of byte-sized types. Otherwise, there are endianness concerns. dotnet/runtime#24961 would be needed and then Roslyn respecting it before it could be used with other types, like the char[] in your repro.

Copy link
Member Author

Choose a reason for hiding this comment

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

Makes sense, didn't think about that optimization only applying to ReadOnlySpan<byte>

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, it's subtle.

var encodedName = Convert.ToBase64String(buffer);
return string.Concat("=?utf-8?B?", encodedName, "?=");
BrennanConroy marked this conversation as resolved.
Show resolved Hide resolved
}

// Attempt to decode MIME encoded strings
Expand Down
72 changes: 10 additions & 62 deletions src/Http/Headers/src/HeaderUtilities.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -325,7 +325,7 @@ public static bool ContainsCacheDirective(StringValues cacheControlDirectives, s
return false;
}

private static unsafe bool TryParseNonNegativeInt64FromHeaderValue(int startIndex, string headerValue, out long result)
private static bool TryParseNonNegativeInt64FromHeaderValue(int startIndex, string headerValue, out long result)
{
// Trim leading whitespace
startIndex += HttpRuleParser.GetWhitespaceLength(headerValue, startIndex);
Expand Down Expand Up @@ -366,40 +366,15 @@ private static unsafe bool TryParseNonNegativeInt64FromHeaderValue(int startInde
/// result will be overwritten.
/// </param>
/// <returns><see langword="true" /> if parsing succeeded; otherwise, <see langword="false" />.</returns>
public static unsafe bool TryParseNonNegativeInt32(StringSegment value, out int result)
public static bool TryParseNonNegativeInt32(StringSegment value, out int result)
{
if (string.IsNullOrEmpty(value.Buffer) || value.Length == 0)
{
result = 0;
return false;
}

result = 0;
fixed (char* ptr = value.Buffer)
{
var ch = (ushort*)ptr + value.Offset;
var end = ch + value.Length;

ushort digit = 0;
while (ch < end && (digit = (ushort)(*ch - 0x30)) <= 9)
{
// Check for overflow
if ((result = result * 10 + digit) < 0)
{
result = 0;
return false;
}

ch++;
}

if (ch != end)
{
result = 0;
return false;
}
return true;
}
return int.TryParse(value.AsSpan(), NumberStyles.None, NumberFormatInfo.InvariantInfo, out result);
}

/// <summary>
Expand All @@ -417,40 +392,14 @@ public static unsafe bool TryParseNonNegativeInt32(StringSegment value, out int
/// originally supplied in result will be overwritten.
/// </param>
/// <returns><see langword="true" /> if parsing succeeded; otherwise, <see langword="false" />.</returns>
public static unsafe bool TryParseNonNegativeInt64(StringSegment value, out long result)
public static bool TryParseNonNegativeInt64(StringSegment value, out long result)
{
if (string.IsNullOrEmpty(value.Buffer) || value.Length == 0)
{
result = 0;
return false;
}

result = 0;
fixed (char* ptr = value.Buffer)
{
var ch = (ushort*)ptr + value.Offset;
var end = ch + value.Length;

ushort digit = 0;
while (ch < end && (digit = (ushort)(*ch - 0x30)) <= 9)
{
// Check for overflow
if ((result = result * 10 + digit) < 0)
{
result = 0;
return false;
}

ch++;
}

if (ch != end)
{
result = 0;
return false;
}
return true;
}
return long.TryParse(value.AsSpan(), NumberStyles.None, NumberFormatInfo.InvariantInfo, out result);
}

// Strict and fast RFC7231 5.3.1 Quality value parser (and without memory allocation)
Expand Down Expand Up @@ -553,7 +502,7 @@ internal static bool TryParseQualityDouble(StringSegment input, int startIndex,
/// <returns>
/// The string representation of the value of this instance, consisting of a sequence of digits ranging from 0 to 9 with no leading zeroes.
/// </returns>
public unsafe static string FormatNonNegativeInt64(long value)
public static string FormatNonNegativeInt64(long value)
{
if (value < 0)
{
Expand All @@ -566,18 +515,17 @@ public unsafe static string FormatNonNegativeInt64(long value)
}

var position = _int64MaxStringLength;
char* charBuffer = stackalloc char[_int64MaxStringLength];
Span<char> charBuffer = stackalloc char[_int64MaxStringLength];
BrennanConroy marked this conversation as resolved.
Show resolved Hide resolved

do
{
// Consider using Math.DivRem() if available
var quotient = value / 10;
charBuffer[--position] = (char)(0x30 + (value - quotient * 10)); // 0x30 = '0'
var (quotient, rem) = Math.DivRem(value, 10);
charBuffer[--position] = (char)(0x30 + rem); // 0x30 = '0'
value = quotient;
}
while (value != 0);

return new string(charBuffer, position, _int64MaxStringLength - position);
return new string(charBuffer.Slice(position));
}

/// <summary>
Expand Down
5 changes: 4 additions & 1 deletion src/Http/Headers/src/MediaTypeHeaderValue.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
using System.Diagnostics.Contracts;
using System.Globalization;
using System.Linq;
using System.Runtime.InteropServices;
using System.Text;
using Microsoft.Extensions.Primitives;

Expand Down Expand Up @@ -645,7 +646,9 @@ private static int GetMediaTypeExpressionLength(StringSegment input, int startIn
}
else
{
mediaType = input.Substring(startIndex, typeLength) + ForwardSlashCharacter + input.Substring(current, subtypeLength);
var forwardSlashCharacter = ForwardSlashCharacter;
var forwardSlash = MemoryMarshal.CreateReadOnlySpan(ref forwardSlashCharacter, 1);
BrennanConroy marked this conversation as resolved.
Show resolved Hide resolved
mediaType = string.Concat(input.AsSpan().Slice(startIndex, typeLength), forwardSlash, input.AsSpan().Slice(current, subtypeLength));
BrennanConroy marked this conversation as resolved.
Show resolved Hide resolved
}

return mediaTypeLength;
Expand Down
3 changes: 1 addition & 2 deletions src/Http/Headers/src/Microsoft.Net.Http.Headers.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,9 @@
<Project Sdk="Microsoft.NET.Sdk">
<Project Sdk="Microsoft.NET.Sdk">

<PropertyGroup>
<Description>HTTP header parser implementations.</Description>
<TargetFramework>$(DefaultNetCoreTargetFramework)</TargetFramework>
<IsAspNetCoreApp>true</IsAspNetCoreApp>
<AllowUnsafeBlocks>true</AllowUnsafeBlocks>
<GenerateDocumentationFile>true</GenerateDocumentationFile>
<PackageTags>http</PackageTags>
<IsPackable>false</IsPackable>
Expand Down
4 changes: 2 additions & 2 deletions src/Http/Routing/src/Matching/SingleEntryAsciiJumpTable.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// Copyright (c) .NET Foundation. All rights reserved.
// Copyright (c) .NET Foundation. All rights reserved.
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

using System;
Expand Down Expand Up @@ -27,7 +27,7 @@ public SingleEntryAsciiJumpTable(
_destination = destination;
}

public unsafe override int GetDestination(string path, PathSegment segment)
public override int GetDestination(string path, PathSegment segment)
{
var length = segment.Length;
if (length == 0)
Expand Down
1 change: 0 additions & 1 deletion src/Http/Routing/src/Microsoft.AspNetCore.Routing.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@ Microsoft.AspNetCore.Routing.RouteCollection</Description>
<IsAspNetCoreApp>true</IsAspNetCoreApp>
<GenerateDocumentationFile>true</GenerateDocumentationFile>
<PackageTags>aspnetcore;routing</PackageTags>
<AllowUnsafeBlocks>true</AllowUnsafeBlocks>
<IsPackable>false</IsPackable>
<Nullable>enable</Nullable>
</PropertyGroup>
Expand Down