Skip to content
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
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
76 changes: 64 additions & 12 deletions sdk/src/Core/Amazon.Util/AWSSDKUtils.cs
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@

using Amazon.Runtime.Internal.Util;
using System;
using System.Buffers;
using System.Collections.Generic;
using System.Globalization;
using System.IO;
Expand Down Expand Up @@ -761,14 +762,59 @@ public static long ConvertTimeSpanToMilliseconds(TimeSpan timeSpan)
/// <returns>String version of the data</returns>
public static string ToHex(byte[] data, bool lowercase)
{
StringBuilder sb = new StringBuilder();

for (int i = 0; i < data.Length; i++)
#if NET8_0_OR_GREATER
if (!lowercase)
{
sb.Append(data[i].ToString(lowercase ? "x2" : "X2", CultureInfo.InvariantCulture));
return Convert.ToHexString(data);
}
#endif

return sb.ToString();
#if NETCOREAPP3_1_OR_GREATER
Func<int, char> converter = lowercase ? (Func<int, char>)ToLowerHex : (Func<int, char>)ToUpperHex;

return string.Create(data.Length * 2, (data, converter), (chars, state) =>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Raised #3316

{
byte[] data = state.data;
Func<int, char> converter = state.converter;

for (int i = data.Length - 1; i >= 0; i--)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@stevenaw stevenaw May 8, 2024

Choose a reason for hiding this comment

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

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.

{
// 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);
}
});
#else
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 = data.Length - 1; i >= 0; 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);
}
#endif
}

/// <summary>
Expand Down Expand Up @@ -1171,7 +1217,18 @@ private static char ToUpperHex(int value)
// 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' (0x61 - 0x66).
return (char)(value - 10 + 'a');
}

internal static string UrlEncodeSlash(string data)
{
if (string.IsNullOrEmpty(data))
Expand Down Expand Up @@ -1333,12 +1390,7 @@ public static void Sleep(TimeSpan ts)
/// </summary>
/// <param name="value">Bytes to convert.</param>
/// <returns>Hexadecimal string representing the byte array.</returns>
public static string BytesToHexString(byte[] value)
{
string hex = BitConverter.ToString(value);
hex = hex.Replace("-", string.Empty);
return hex;
}
public static string BytesToHexString(byte[] value) => ToHex(value, false);
Copy link
Member

Choose a reason for hiding this comment

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

Not sure why we had both ToHex and BytesToHexString. Going with old codebase. The BytesToHexString is only called in AmazonS3ResponseHandler.cs. Since this is v4 I think we can just get rid of this method and update AmazonS3ReponseHandler.


/// <summary>
/// Convert a hex string to bytes
Expand Down
40 changes: 40 additions & 0 deletions sdk/test/NetStandard/UnitTests/Core/AWSSDKUtilsTests.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
using Amazon.Runtime;
using Amazon.Util;
using System;
using System.Collections.Generic;
using System.Text;
using Xunit;

namespace UnitTests.NetStandard.Core
{
[Trait("Category", "Core")]
public class AWSSDKUtilsTests
Copy link
Member

Choose a reason for hiding this comment

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

Since the #else block in the ToHex method is really just for .NET Framework can you copy the tests to the .NET Framework unit tests in the sdk\test\UnitTests\Custom folder. Hopefully as part of V4 we can do some test restructure cleanup so we don't have to copy the test between the .NET Framework and .NET Standard+.

{
[Fact]
public void BytesToHexString()
{
var bytes = Encoding.UTF8.GetBytes("Hello World");
var hexString = AWSSDKUtils.BytesToHexString(bytes);

Assert.Equal("48656C6C6F20576F726C64", hexString);
}

[Fact]
public void ToHexUppercase()
{
var bytes = Encoding.UTF8.GetBytes("Hello World");
var hexString = AWSSDKUtils.ToHex(bytes, false);

Assert.Equal("48656C6C6F20576F726C64", hexString);
}

[Fact]
public void ToHexLowercase()
{
var bytes = Encoding.UTF8.GetBytes("Hello World");
var hexString = AWSSDKUtils.ToHex(bytes, true);

Assert.Equal("48656c6c6f20576f726c64", hexString);
}
}
}