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

Improve Encoding.UTF8.GetMaxByte/CharCount perf #69910

Merged
merged 4 commits into from
Jun 3, 2022

Conversation

GrabYourPitchforks
Copy link
Member

Mostly a very small perf improvement to GetMaxByteCount and GetMaxCharCount. It's now small enough to inline into callers when using the Encoding.UTF8 accelerator.

Also addresses:

  • Adds integer overflow protection to some unmanaged callers.
  • Improves unit test coverage.
  • Cleans up some XML usage.
Method Job Toolchain input Mean Error StdDev Ratio RatioSD
GetTranscodedLenth Job-LLVNSR baseline Hello! 15.27 ns 0.383 ns 1.123 ns 1.00 0.00
GetTranscodedLenth Job-DMFSKR compare Hello! 13.51 ns 0.306 ns 0.340 ns 0.87 0.06
[Benchmark]
[Arguments("Hello!")]
public int GetTranscodedLenth(string input)
{
    const int MaxStackLength = 64;
    byte[] rentedArray = null;
    int maxByteCount = Encoding.UTF8.GetMaxByteCount(input.Length);
    Span<byte> scratch = ((uint)maxByteCount <= MaxStackLength)
        ? stackalloc byte[MaxStackLength]
        : (rentedArray = ArrayPool<byte>.Shared.Rent(maxByteCount));

    try
    {
        return Encoding.UTF8.GetBytes(input, scratch);
    }
    finally
    {
        if (rentedArray != null)
        {
            ArrayPool<byte>.Shared.Return(rentedArray);
        }
    }
}

Also fixes some potential integer overflows in callers
@ghost
Copy link

ghost commented May 27, 2022

Tagging subscribers to this area: @dotnet/area-system-text-encoding
See info in area-owners.md if you want to be subscribed.

Issue Details

Mostly a very small perf improvement to GetMaxByteCount and GetMaxCharCount. It's now small enough to inline into callers when using the Encoding.UTF8 accelerator.

Also addresses:

  • Adds integer overflow protection to some unmanaged callers.
  • Improves unit test coverage.
  • Cleans up some XML usage.
Method Job Toolchain input Mean Error StdDev Ratio RatioSD
GetTranscodedLenth Job-LLVNSR baseline Hello! 15.27 ns 0.383 ns 1.123 ns 1.00 0.00
GetTranscodedLenth Job-DMFSKR compare Hello! 13.51 ns 0.306 ns 0.340 ns 0.87 0.06
[Benchmark]
[Arguments("Hello!")]
public int GetTranscodedLenth(string input)
{
    const int MaxStackLength = 64;
    byte[] rentedArray = null;
    int maxByteCount = Encoding.UTF8.GetMaxByteCount(input.Length);
    Span<byte> scratch = ((uint)maxByteCount <= MaxStackLength)
        ? stackalloc byte[MaxStackLength]
        : (rentedArray = ArrayPool<byte>.Shared.Rent(maxByteCount));

    try
    {
        return Encoding.UTF8.GetBytes(input, scratch);
    }
    finally
    {
        if (rentedArray != null)
        {
            ArrayPool<byte>.Shared.Return(rentedArray);
        }
    }
}
Author: GrabYourPitchforks
Assignees: -
Labels:

area-System.Text.Encoding

Milestone: 7.0.0

}
}

return byteCount + 1;
Copy link
Member

Choose a reason for hiding this comment

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

  • 1

could you remind me why we are adding 1 here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Some callers expect GetMaxCharCount(byteCount) to be the maximum number of characters that might be converted from any call to Encoding.GetDecoder().GetChars(buffer_of_byteCount_length, some_output_buffer). StreamReader makes this assumption, for instance.

DecoderNLS can hold partial state between calls to GetChars if a non-ASCII byte is seen. There are two possible outcomes here:

  1. The internal state is never completed and represents the maximum invalid subsequence of a UTF-8 buffer. The Encoding instance will replace the entire captured state with a single '\uFFFD' character before processing the rest of the input buffer.

  2. The internal state is 3 bytes of a 4-byte sequence, and the first byte of the incoming buffer would complete the sequence. This means the output would contain 2 characters: the high & low surrogates.

In both scenarios, the worst-case expansion is that the internally captured state results in +1 additional character needed in the output.

Copy link
Member

Choose a reason for hiding this comment

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

Do we need more comment here explaining that?

[InlineData(int.MaxValue)]
public void GetMaxByteCount_NegativeTests(int charCount)
{
Assert.Throws<ArgumentOutOfRangeException>(nameof(charCount), () => Encoding.UTF8.GetMaxByteCount(charCount));
Copy link
Member

Choose a reason for hiding this comment

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

As for any Assert.Throws for an ArgumentException, you may choose to prefer the AssertExtensions version that validates the param name as well.

@GrabYourPitchforks
Copy link
Member Author

I updated the code comments in UTF8Encoding to more accurately describe why the +1 is important. Latest iteration is a comment-only change; no functional changes from previous iteration.

Copy link
Member

@tarekgh tarekgh left a comment

Choose a reason for hiding this comment

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

LGTM!

@GrabYourPitchforks
Copy link
Member Author

Build & test failures are known, per runfo.

@GrabYourPitchforks GrabYourPitchforks merged commit be81ac6 into dotnet:main Jun 3, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Jul 3, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants