Add methods to convert between hexadecimal strings and bytes #37546

tkp1n · 2020-06-06T18:31:47Z

This adds two methods (with overloads) ToHexString and FromHexString to System.Convert.

As discussed in the API review and Twitter, the implementation is kept minimal and straight-forward with major optimizations left for a potential follow-up PR.

On HexConverter

I tried to re-use and extend the (internal) HexConverter for the actual implementation.
Both HexConverter.ToString and HexConverter.TryDecodeFromUtf16 could easily be optimized using SIMD, but this is intentionally left for a future PR to focus on getting the feature in first. HexConverter.ToString could be substantially improved using "only" scalar code (see "Numbers" below). I assume, however, that it is written the way it is (branch-free) for a reason.

Numbers

For those interested in performance numbers: I've created some benchmarks comparing different implementations here (including results)

Consolidation

Across the libraries, one can find various different classes and methods handling hex in more or less creative ways. This PR moves a lot of those to the implementations to HexConverter.

Dotnet-GitSync-Bot · 2020-06-06T18:31:52Z

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

vcsjones · 2020-06-06T20:03:45Z

I missed the API review for this one, was there any discussion or thought about ignoring whitespace / newlines like the To/FromBase64String APIs?

tkp1n · 2020-06-06T20:10:17Z

I only recall the decision to only output uppercase/no-whitespace... Nothing about the FromHexString side of things... that's why I'm bringing it up.

Yes, the Base64 From*-methods accept whitespace (to some extent), but that's likely because the respective To*-methods produce whitespace in their output.

src/libraries/System.Private.CoreLib/src/System/Convert.cs

stephentoub · 2020-06-07T15:13:42Z

uppercase only, no white-space

It should definitely parse lowercase as well.

tkp1n · 2020-06-07T16:08:04Z

It should definitely parse lowercase as well.

Done: 71ee4e1

GSPP · 2020-06-08T08:07:45Z

If the check ToBase64_CalculateAndValidateOutputLength is required for correctness then maybe it should test for the maximum string/array length which is less than int.MaxValue.

tkp1n · 2020-06-09T10:22:56Z

@GrabYourPitchforks Your thoughts on the topic of supporting white-space input in FromHexString would be highly appreciated.

Also, do you (or someone else for that matter) know who to ask for support regarding the crossgen build issue?

vcsjones · 2020-06-09T12:40:56Z

/cc @nattress this PR is failing because crossgen errors on build. Are you the right person to tag for help?

tkp1n · 2020-06-30T13:22:19Z

@GrabYourPitchforks, @stephentoub: could you please take a look again and help push this across the finish line?

src/libraries/Common/src/System/HexConverter.cs

stephentoub · 2020-06-30T16:27:00Z

src/libraries/System.Runtime/ref/System.Runtime.cs

+        public static byte[] FromHexString(System.ReadOnlySpan<char> chars) { throw null; }
+        public static string ToHexString(byte[] inArray) { throw null; }
+        public static string ToHexString(byte[] inArray, int offset, int length) { throw null; }
+        public static string ToHexString(System.ReadOnlySpan<byte> bytes) { throw null; }


Are there any places in the dotnet/runtime libraries where we're currently doing these operations manually such that they could instead be switched to call these APIs?

Most byte -> string conversions have already consolidated to use the internal HexConverter as part of Unify int to hexadecimal char conversions #1273. Some places could however profit from the new APIs (e.g. src\libraries\Common\tests\System\Security\Cryptography\ByteUtils.cs)

There are a few string -> byte conversions across the libraries. Some (e.g. ActivityTraceId.HexDigitToBinary) are, however, case sensitive...

Would you like to see a cleanup/consolidation as part of this PR or as a follow-up?

My question stems more from wanting to make sure the methods are actually useful (that's a question about the approved API design rather than about your work in the PR). If with all of the places we do hex conversions we don't actually have any places that would benefit from these APIs, it makes me question whether they're the right APIs to be adding. As such, I would like to see in this PR us switching call sites that could/should use them to do so, so that we can see that they are actually valuable and get it all done in one go.

cc: @bartonjs, @GrabYourPitchforks

Well, my ByteArrayToHex() methods could certainly be replaced with this... if all of my compilation targets were net5+ (I don't remember if I have tests that run on netfx anymore).

There's some hex parsing and hex producing code in the crypto libraries themselves... the parser ignores any non-hex data (at this point, for compat; historically, because... uh... clearly it was whitespace?) so probably can't change. The hex producers, possibly... if they're only used in net5 targets (the Pkcs and Xml libraries are netstandard OOBs, but I feel like the hex producing code is in S.S.C.Encoding, which is in the framework, so it might be able to change, unless it does weird things with whitespace).

@stephentoub An analysis of the usefulness of the new API for use within the dotnet/runtime libraries

Could profit from new API (ignoring potential issues with compilation targets):

src\libraries\Common\tests\System\Security\Cryptography\ByteUtils.cs:25 src\libraries\Common\tests\System\Security\Cryptography\ByteUtils.cs:53 src\libraries\System.DirectoryServices.AccountManagement\src\System\DirectoryServices\AccountManagement\Utils.cs:87 src\libraries\System.Drawing.Common\tests\mono\System.Drawing\BitmapTests.cs:393 src\libraries\System.Net.NetworkInformation\src\System\Net\NetworkInformation\PhysicalAddress.cs:93 src\libraries\System.Reflection.MetadataLoadContext\tests\src\TestUtils\TestUtils.cs:108 src\libraries\System.Reflection.MetadataLoadContext\tests\src\TestUtils\TestUtils.cs:121 src\libraries\System.Runtime\tests\System\Text\Unicode\Utf8Tests.cs:64 src\libraries\System.Security.Cryptography.Encoding\src\Internal\Cryptography\AsnFormatter.cs:15 src\libraries\System.Security.Cryptography.X509Certificates\src\Internal\Cryptography\Helpers.cs:36 src\libraries\System.Windows.Extensions\tests\X509Certificate2UIManualTests.cs:73

Could profit from making access to the lookup table in HexConverter public:

src\libraries\System.Security.Cryptography.X509Certificates\src\Internal\Cryptography\Helpers.cs:98

Cannot profit from the new methods due to limitations of the new API

src\libraries\Common\tests\System\Net\Http\LoopbackServer.AuthenticationHelpers.cs:287 - Encode lowercase src\libraries\System.Diagnostics.DiagnosticSource\src\System\Diagnostics\Activity.cs - Encode lowercase src\libraries\System.DirectoryServices.AccountManagement\src\System\DirectoryServices\AccountManagement\Utils.cs:75 - Encode lowercase src\libraries\System.Private.CoreLib\src\System\ApplicationId.cs:67 - Decode to Span<char> src\libraries\System.Runtime.Numerics\tests\BigInteger\MyBigInt.cs:903 - Decode lowercase, space-separated src\libraries\System.Security.Cryptography.Encoding\src\Internal\Cryptography\AsnFormatter.cs:22 - Encode space-separated src\libraries\System.Security.Cryptography.Pkcs\src\Internal\Cryptography\PkcsHelpers.cs:348 - Decode (enforce uppercase) src\libraries\System.Security.Cryptography.X509Certificates\src\Internal\Cryptography\Helpers.cs:24 - Decode to Span<char> src\libraries\System.Security.Cryptography.X509Certificates\src\Internal\Cryptography\Helpers.cs:41 - Decode white space

It could be that I missed a few additional places...

Thanks for the analysis!

@bartonjs, @GrabYourPitchforks, we still believe the APIs approved and implemented publicly in this PR are the right ones?

Personally, I'd want to see at least at least a couple of existing call sites switched in this PR, or else I question the real worthwhileness of the particular shapes employed here. We already have BitConverter.ToString which outputs at least one pattern; this (at least the ToString direction) is just outputting another form. I question whether this form is so much more useful that it's worth adding.

we still believe the APIs approved and implemented publicly in this PR are the right ones?

I don't remember why we didn't also do the span-writing versions, but these are definitely better than (leads into)

We already have BitConverter.ToString which outputs at least one pattern; this (at least the ToString direction) is just outputting another form.

While I use that one in a pinch, I generally don't want the hyphens. The top hit on StackOverflow for "BitConverter.ToString" is

public static string ByteArrayToString(byte[] ba) { return BitConverter.ToString(ba).Replace("-",""); }

Suggesting that the form here is what many (if not most) callers want. So I feel there's definite goodness.

src/libraries/System.Runtime.Extensions/tests/System/Convert.ToHexString.cs

stephentoub

Thanks for working on this.

tkp1n · 2020-07-01T08:41:16Z

@nattress sorry to bother you again with this PR...
Could you please have a look at the crossgen build error in this PR? Any help would be highly appreciated

nattress · 2020-07-01T19:01:25Z

Crossgen fails because it couldn't find a type while compiling System.Private.CoreLib:

Error compiling F:\workspace\_work\1\s\src\coreclr\..\..\artifacts\bin\coreclr\Windows_NT.x86.Checked\IL\System.Private.CoreLib.dll: Could not load type 'System.Collections.Generic.UInt32EnumComparer`1' from assembly 'System.Private.CoreLib, Version=5.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e'.

That type was renamed two years ago (dotnet/coreclr@82e02f3) so it looks like we need to update

runtime/src/coreclr/src/vm/compile.cpp

Line 4843 in d69cfa0

CorElementType et = elemTypeHnd.GetVerifierCorElementType();

accordingly. Not sure why this hasn't been a problem until now though.

tkp1n · 2020-07-01T21:02:12Z

@nattress

That type was renamed two years ago so it looks like we need to update SpecializeComparer accordingly

Thank you for your help. I attempted to fix this over at #38681

tkp1n · 2020-07-16T08:15:43Z

It appears to me as if the CI failures are not related to the changes in this PR. Please correct me if I'm wrong though!
So unless there is more review feedback this would be it from my side..

@stephentoub let me know if there is anything else I could do here and how you'd like to proceed with your concerns regarding the public API.

bartonjs · 2020-07-17T00:25:24Z

On the one hand, I don't understand any of the test failures. On the other, they repeated on a rerun and I haven't seen them on other PRs. Maybe something internal had a weird hex parsing or formatting going on and it's causing a complicated test failure.

The only way I know to get a nice clean run is to do either push an empty commit or (more useful, probably) merge in the master branch and let it run with the new commit state.

@GrabYourPitchforks You work in this layer more than I do, do the test failures at https://dev.azure.com/dnceng/public/_build/results?buildId=730614&view=ms.vss-test-web.build-test-results-tab&runId=22724830&resultId=102547&paneView=debug make any sense to you?

bartonjs · 2020-07-17T17:07:50Z

/azp run runtime

steveharter · 2020-07-17T17:12:31Z

/azp list

azure-pipelines · 2020-07-17T17:12:45Z

steveharter · 2020-07-17T17:18:36Z

/azp run

azure-pipelines · 2020-07-17T17:18:51Z

You have several pipelines (over 10) configured to build pull requests in this repository. Specify which pipelines you would like to run by using /azp run [pipelines] command. You can specify multiple pipelines using a comma separated list.

steveharter · 2020-07-17T17:19:04Z

/azp run runtime

azure-pipelines · 2020-07-17T17:19:38Z

Azure Pipelines successfully started running 1 pipeline(s).

bartonjs · 2020-07-17T17:53:36Z

/azp run dotnet-linker-tests,dotnet-runtime-perf

bartonjs · 2020-07-17T19:10:38Z

/azp run dotnet-linker-tests

azure-pipelines · 2020-07-17T19:11:22Z

Azure Pipelines successfully started running 1 pipeline(s).

bartonjs · 2020-07-18T00:09:30Z

The failures look to be known: #39484.

Dotnet-GitSync-Bot added area-System.Runtime new-api-needs-documentation labels Jun 6, 2020

vcsjones reviewed Jun 6, 2020

View reviewed changes

src/libraries/System.Private.CoreLib/src/System/Convert.cs Outdated Show resolved Hide resolved

Youssef1313 reviewed Jun 7, 2020

View reviewed changes

src/libraries/System.Private.CoreLib/src/System/Convert.cs Outdated Show resolved Hide resolved

Youssef1313 reviewed Jun 7, 2020

View reviewed changes

src/libraries/System.Private.CoreLib/src/System/Convert.cs Outdated Show resolved Hide resolved

tkp1n requested a review from stephentoub June 22, 2020 14:02

stephentoub reviewed Jun 30, 2020

View reviewed changes

src/libraries/Common/src/System/HexConverter.cs Show resolved Hide resolved

stephentoub reviewed Jun 30, 2020

View reviewed changes

src/libraries/System.Runtime.Extensions/tests/System/Convert.ToHexString.cs Outdated Show resolved Hide resolved

stephentoub reviewed Jun 30, 2020

View reviewed changes

tkp1n mentioned this pull request Jul 1, 2020

Align native SpecializeComparer with managed ComparerHelpers #38681

Merged

tkp1n added 4 commits July 2, 2020 08:34

Add methods to convert between hexadecimal strings and bytes

2753cb6

Support lowercase decoding

3c96756

Change arithmetics around ROS.Length to help the JIT produce better code

b546388

Address review feedback

3f2b589

tkp1n mentioned this pull request Jul 2, 2020

Use full MD5 to compare bitmap rotate results #38699

Merged

tkp1n added 2 commits July 2, 2020 10:58

Reuse existing CharToHexLookup table

f54ef1e

Migrate call sites to new API

044c710

jaredpar mentioned this pull request Jul 14, 2020

Errors installing the SDK during builds #34015

Closed

tkp1n added 2 commits July 15, 2020 10:20

Address review feedback

de2085b

Remove another duplicate hex lookup table

7917e6d

Merge remote-tracking branch 'upstream/master' into hex

8c89198

bartonjs closed this Jul 17, 2020

bartonjs reopened this Jul 17, 2020

bartonjs approved these changes Jul 18, 2020

View reviewed changes

bartonjs merged commit f6dc87f into dotnet:master Jul 18, 2020

tkp1n deleted the hex branch July 20, 2020 08:10

jeffhandley mentioned this pull request Jul 20, 2020

[release/5.0-preview8] Add methods to convert between hexadecimal strings and bytes (#37546) #39658

Merged

tkp1n mentioned this pull request Jul 21, 2020

Add vectorized implementation of hex encoding/decoding #39702

Closed

wfurt mentioned this pull request Jul 22, 2020

[Perf -19%] System.Net.NetworkInformation.Tests.PhysicalAddressTests.PAShort #39720

Closed

kunalspathak mentioned this pull request Jul 24, 2020

[Perf -35%] System.Memory.Span<Char>.IndexOfValue #39722

Closed

tkp1n mentioned this pull request Aug 9, 2020

Fix perf regression in System.Net.NetworkInformation.PhysicalAddress #40571

Closed

wfurt mentioned this pull request Sep 29, 2020

Regression - NET5.0 RC1 returns empty MAC address for network interface on Linux #42870

Closed

qmfrederik mentioned this pull request Nov 21, 2020

Add a managed Git implementation dotnet/Nerdbank.GitVersioning#521

Merged

12 tasks

ghost locked as resolved and limited conversation to collaborators Dec 8, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add methods to convert between hexadecimal strings and bytes #37546

Add methods to convert between hexadecimal strings and bytes #37546

tkp1n commented Jun 6, 2020 •

edited

Loading

Dotnet-GitSync-Bot commented Jun 6, 2020

vcsjones commented Jun 6, 2020

tkp1n commented Jun 6, 2020

stephentoub commented Jun 7, 2020

tkp1n commented Jun 7, 2020

GSPP commented Jun 8, 2020

tkp1n commented Jun 9, 2020

vcsjones commented Jun 9, 2020

tkp1n commented Jun 30, 2020

stephentoub Jun 30, 2020

tkp1n Jun 30, 2020

stephentoub Jul 1, 2020 •

edited

Loading

bartonjs Jul 1, 2020

tkp1n Jul 1, 2020 •

edited

Loading

stephentoub Jul 10, 2020

bartonjs Jul 16, 2020

stephentoub left a comment

tkp1n commented Jul 1, 2020

nattress commented Jul 1, 2020

tkp1n commented Jul 1, 2020

tkp1n commented Jul 16, 2020

bartonjs commented Jul 17, 2020

bartonjs commented Jul 17, 2020

steveharter commented Jul 17, 2020

azure-pipelines bot commented Jul 17, 2020

steveharter commented Jul 17, 2020

azure-pipelines bot commented Jul 17, 2020

steveharter commented Jul 17, 2020

azure-pipelines bot commented Jul 17, 2020

bartonjs commented Jul 17, 2020

bartonjs commented Jul 17, 2020

azure-pipelines bot commented Jul 17, 2020

bartonjs commented Jul 18, 2020

Add methods to convert between hexadecimal strings and bytes #37546

Add methods to convert between hexadecimal strings and bytes #37546

Conversation

tkp1n commented Jun 6, 2020 • edited Loading

On HexConverter

Numbers

Consolidation

Dotnet-GitSync-Bot commented Jun 6, 2020

vcsjones commented Jun 6, 2020

tkp1n commented Jun 6, 2020

stephentoub commented Jun 7, 2020

tkp1n commented Jun 7, 2020

GSPP commented Jun 8, 2020

tkp1n commented Jun 9, 2020

vcsjones commented Jun 9, 2020

tkp1n commented Jun 30, 2020

stephentoub Jun 30, 2020

Choose a reason for hiding this comment

tkp1n Jun 30, 2020

Choose a reason for hiding this comment

stephentoub Jul 1, 2020 • edited Loading

Choose a reason for hiding this comment

bartonjs Jul 1, 2020

Choose a reason for hiding this comment

tkp1n Jul 1, 2020 • edited Loading

Choose a reason for hiding this comment

stephentoub Jul 10, 2020

Choose a reason for hiding this comment

bartonjs Jul 16, 2020

Choose a reason for hiding this comment

stephentoub left a comment

Choose a reason for hiding this comment

tkp1n commented Jul 1, 2020

nattress commented Jul 1, 2020

tkp1n commented Jul 1, 2020

tkp1n commented Jul 16, 2020

bartonjs commented Jul 17, 2020

bartonjs commented Jul 17, 2020

steveharter commented Jul 17, 2020

azure-pipelines bot commented Jul 17, 2020

steveharter commented Jul 17, 2020

azure-pipelines bot commented Jul 17, 2020

steveharter commented Jul 17, 2020

azure-pipelines bot commented Jul 17, 2020

bartonjs commented Jul 17, 2020

bartonjs commented Jul 17, 2020

azure-pipelines bot commented Jul 17, 2020

bartonjs commented Jul 18, 2020

tkp1n commented Jun 6, 2020 •

edited

Loading

stephentoub Jul 1, 2020 •

edited

Loading

tkp1n Jul 1, 2020 •

edited

Loading