Bytes -> Hex conversion update

[rudzen]

Bytes -> Hex conversion update

Hi!

I've updated the functionality which converts a byte array to a lower case hex string.

The performance is improved while still being compatible with the previous version.
For good measure I've added a few unit tests for SHA-1 hashing which uses the wiki examples.

Changes

• Added new byte-to-hex compute functionality
• Added sha-1 unit tests which computes hashes from wiki hash examples

Affects

Anything that computes a SHA1 hash through CryptoUtil.

Benchmarks

To get an idea of the scaling, I cooked up a little benchmark to test the changes when hashing with SHA-1.

Runs with data sizes of 32 and 128 data sizes.

BenchmarkDotNet v0.13.8, Windows 11 (10.0.22621.2283/22H2/2022Update/SunValley2)
Intel Core i7-8086K CPU 4.00GHz (Coffee Lake), 1 CPU, 12 logical and 6 physical cores
.NET SDK 7.0.401
  [Host]     : .NET 6.0.22 (6.0.2223.42425), X64 RyuJIT AVX2
  DefaultJob : .NET 6.0.22 (6.0.2223.42425), X64 RyuJIT AVX2

| Method       | source    | Mean       | Error    | StdDev   | Ratio | RatioSD | Gen0   | Allocated | Alloc Ratio |
|------------- |---------- |-----------:|---------:|---------:|------:|--------:|-------:|----------:|------------:|
| New          | Byte[32]  |   572.1 ns |  3.98 ns |  3.53 ns |  1.00 |    0.00 | 0.0515 |     328 B |        1.00 |
| Mono         | Byte[32]  |   584.4 ns |  6.29 ns |  5.89 ns |  1.02 |    0.01 | 0.0687 |     432 B |        1.32 |
| Convert      | Byte[32]  |   584.6 ns |  7.32 ns |  6.49 ns |  1.02 |    0.01 | 0.0687 |     432 B |        1.32 |
| New          | Byte[128] |   763.4 ns |  6.32 ns |  5.60 ns |  1.33 |    0.01 | 0.0515 |     328 B |        1.00 |
| Convert      | Byte[128] |   779.6 ns |  6.37 ns |  5.65 ns |  1.36 |    0.01 | 0.0687 |     432 B |        1.32 |
| Mono         | Byte[128] |   788.1 ns |  7.79 ns |  7.29 ns |  1.38 |    0.01 | 0.0687 |     432 B |        1.32 |
| BitConverter | Byte[32]  |   952.5 ns |  6.68 ns |  5.92 ns |  1.66 |    0.02 | 0.0916 |     576 B |        1.76 |
| BitConverter | Byte[128] | 1,154.8 ns |  3.34 ns |  2.79 ns |  2.02 |    0.01 | 0.0916 |     576 B |        1.76 |
| Old          | Byte[32]  | 1,979.5 ns | 20.45 ns | 17.07 ns |  3.46 |    0.03 | 0.3357 |    2120 B |        6.46 |
| Old          | Byte[128] | 2,167.2 ns | 15.33 ns | 14.34 ns |  3.79 |    0.04 | 0.3357 |    2120 B |        6.46 |

[PunkPun]

👋

it seems that this solution is incompatible with the mono build

OpenRA.Game/CryptoUtil.cs(283,40): error CS0117: 'MemoryMarshal' does not contain a definition for 'GetArrayDataReference'

[RoosterDragon]

You can guard with #if NET6_0_OR_GREATER and keep the previous implementation around for mono. Alternatively, implementing the loop without any unsafe might sidestep the problem and still be faster.

How does this method compare to Convert.ToHexString (which would need #if NET5_0_OR_GREATER) followed by a lowercase?

[rudzen]

You can guard with #if NET6_0_OR_GREATER and keep the previous implementation around for mono. Alternatively, implementing the loop without any unsafe might sidestep the problem and still be faster.

How does this method compare to Convert.ToHexString (which would need #if NET5_0_OR_GREATER) followed by a lowercase?

Added an alternate implementation for mono using #if NET5_0_OR_GREATER, hopefully that should work.
I tried out some other variations, including the Convert.ToHexString() and also BitConverter.ToString() but they seem pretty cumbersome in comparison because of the lowercase and/or '-' removal.

Did not know I could bump to the #if NET6_0_OR_GREATER. Will keep that in mind :)

[RoosterDragon]

This reduces time spent in SHA1 hashing during initial load from ~1.4% to 1.2%, which is nice incremental win.

The downside for me is the maintenace burden here. We've added a fair bit of extra code, had to deal with an alternative implementation for mono and added unsafe code. For some larger overall gains I'd say that is worthwhile, but a fraction of a percent in the project overall doesn't sell it for me. Not your fault by any means, if the project was doing like 10-100x more hashing it'd be more tempting.

In this particular case I think you can get a more favourable balance by investigating Convert.ToHexString or BitConverter.ToString. Since they'd be no more complex than the current code, it'd be an easy sell even if the performance win isn't quite as good.

[PunkPun]

The only other place where we use x2 format is in color code. But it's not hotly used as well

[rudzen]

This reduces time spent in SHA1 hashing during initial load from ~1.4% to 1.2%, which is nice incremental win.

The downside for me is the maintenace burden here. We've added a fair bit of extra code, had to deal with an alternative implementation for mono and added unsafe code. For some larger overall gains I'd say that is worthwhile, but a fraction of a percent in the project overall doesn't sell it for me. Not your fault by any means, if the project was doing like 10-100x more hashing it'd be more tempting.

In this particular case I think you can get a more favourable balance by investigating Convert.ToHexString or BitConverter.ToString. Since they'd be no more complex than the current code, it'd be an easy sell even if the performance win isn't quite as good.

Using Convert.ToHexString yields almost the same result (see updated benchmark table) and should hardly be noticeable 😄.

[PunkPun]

OpenRA/OpenRA.Game/CryptoUtil.cs(269,19): error CS0117: 'Convert' does not contain a definition for 'ToHexString'

it seems it's incompatible with mono 😩

[rudzen]

OpenRA/OpenRA.Game/CryptoUtil.cs(269,19): error CS0117: 'Convert' does not contain a definition for 'ToHexString'

it seems it's incompatible with mono 😩

It would seem mono doesn't like me that much 😢.
Updated it anyways with a simple none-pre processor version that should work just fine.

[PunkPun]

LGTM, could you squash the commits?

[rudzen]

LGTM, could you squash the commits?

👍🏻

[PunkPun]

Chanelog