Optimize AWSSDKUtils.ToHex() for speed and memory

[stevenaw]

Description

Optimize uppercase and lowercase versions of ToHex() across all runtimes. Use the built-in option for uppercase on .NET8. Improved speed + memory, particularly for uppercase on .NET8. There's still more room for improvement for the other scenarios but I decided to keep it simple and follow the existing bit twiddling patterns I saw in UrlEncode in the file

Motivation and Context

Testing

Added unit tests for upper + lower case prior to making change. Ran them at the end to validate the new implementation passed. Ran benchmarks against netcoreapp3.1 and net8.0

Screenshots (if appropriate)

Benchmark Code

    [MemoryDiagnoser]
  public class ToHexString
  {
      // 11 = "Hello World" :)
      [Params(11, 128)]
      public int N { get; set; }

      // 11 = "Hello World" :)
      [Params(true, false)]
      public bool Lowercase { get; set; }

      public byte[] Payload { get; set; }

      [GlobalSetup]
      public void Setup()
      {
          Payload = Encoding.UTF8.GetBytes(new string('A', N));
      }

      [Benchmark(Baseline = true)]
      public string Original()
      {
          var data = Payload;
          var lowercase = Lowercase;

          StringBuilder sb = new StringBuilder();

          for (int i = 0; i < data.Length; i++)
          {
              sb.Append(data[i].ToString(lowercase ? "x2" : "X2", CultureInfo.InvariantCulture));
          }

          return sb.ToString();
      }

      [Benchmark]
      public string Optimized()
      {
          var data = Payload;
          var lowercase = Lowercase;

#if NET8_0_OR_GREATER
          if (!lowercase)
          {
              return Convert.ToHexString(data);
          }
#endif

          char[] chars = ArrayPool<char>.Shared.Rent(data.Length * 2);

          try
          {
              Func<int, char> converter = lowercase ? (Func<int, char>)ToLowerHex : (Func<int, char>)ToUpperHex;

              for (int i = 0; i < data.Length; i++)
              {
                  // Break apart the byte into two four-bit components and
                  // then convert each into their hexadecimal equivalent.
                  byte b = data[i];
                  int hiNibble = b >> 4;
                  int loNibble = b & 0xF;

                  chars[i * 2] = converter(hiNibble);
                  chars[i * 2 + 1] = converter(loNibble);
              }

              return new string(chars, 0, data.Length * 2);
          }
          finally
          {
              ArrayPool<char>.Shared.Return(chars);
          }
      }
      private static char ToUpperHex(int value)
      {
          // Maps 0-9 to the Unicode range of '0' - '9' (0x30 - 0x39).
          if (value <= 9)
          {
              return (char)(value + '0');
          }
          // Maps 10-15 to the Unicode range of 'A' - 'F' (0x41 - 0x46).
          return (char)(value - 10 + 'A');
      }

      private static char ToLowerHex(int value)
      {
          // Maps 0-9 to the Unicode range of '0' - '9' (0x30 - 0x39).
          if (value <= 9)
          {
              return (char)(value + '0');
          }
          // Maps 10-15 to the Unicode range of 'A' - 'F' (0x41 - 0x46).
          return (char)(value - 10 + 'a');
      }
  }

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)

Checklist

• My code follows the code style of this project
• My change requires a change to the documentation
• I have updated the documentation accordingly
• I have read the README document
• I have added tests to cover my changes
• All new and existing tests passed

License

• I confirm that this pull request can be released under the Apache 2 license

[danielmarbach]

FYI I also added a similar unit test in this draft PR that optimizes UrlEncode that you mention here #3307

There I also removed Netstandard from the unit test since that is not a valid target for unit tests.

[danielmarbach]

I just stumbled into this because I was tweaking UrlEncode. Great stuff 😍

[danielmarbach]

This should probably move outside the #if

[danielmarbach]

Out of curiosity have you tried reversing the loop to elide bound checks?

Also could this be a candidate for string.Create since that would take care of the pooling already?

[stevenaw]

Thanks for the suggestions @danielmarbach ! Both were very helpful.

[stevenaw]

One note: I had to keep this lambda non-static in order to be compatible with <LangVersion>8</LangVersion> in the csproj.

[danielmarbach]

This should be changed in my opinion. A reasonable version should at least be 9 even when targeting Netstandard 2.0. That is a good language subset without exposing yourself to weird edge cases. That's how also the Azure .NET SDK treated it until they changed it to an even later version as far as I recall.

[danielmarbach]

Raised #3316

[stevenaw]

One note: I found two copies of the loops here were required to get the best performance as they each operate on different types. Trying to funnel both paths through a Span<byte> to consolidate the code ended up ~10% slower when consumed by .NET Framework.

I'm not sure if any reviewers knew of tricks to get around this without hitting "slow span" paths there.

[danielmarbach]

My experience is that even if you end up with slow downs in the microbenchmarks without a reasonable range due to the massive allocation reductions the overall beneficial effects of getting rid of the allocations especially in a real system using some form of concurrency will outweigh the small latency hits. It is not worth the complexity of maintaining different paths for accommodate for the slow span path. This also takes into account that more modern .NET versions are becoming more and more mainstream and people that are on .NET Framework know what they are not getting.

[stevenaw]

Thanks for the prompt feedback and advice here @danielmarbach . That makes perfect sense to me.
I've pushed another changeset. My laptop is on battery at the moment so benchmarking will be less reliable, but I can try to run and share them later tonight.

EDIT: It seems the csproj changes mean I now have conflicts. Looks like a rebase is ahead.

[stevenaw]

I've pushed a new commit to incorporate the first review feedback. I've rerun the benchmarks, now slightly modified to focus on 128 length. Eliding the bounds check seems to have saved around 20ns.