Convert.Try{From/To}HexString by hrrrrustic · Pull Request #86556 · dotnet/runtime

hrrrrustic · 2023-05-21T16:34:13Z

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

gfoidl · 2023-05-21T19:17:56Z

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

+        /// <returns>true if the conversion was successful; otherwise, false.</returns>
+        public static bool TryFromHexString(ReadOnlySpan<char> source, Span<byte> destination, out int bytesWritten)
+        {
+            var length = source.Length;


Suggested change

var length = source.Length;

int length = source.Length;

Use concrete types here. Same on other places.

Perf-wise this could be nuint, so all the division and modulo operations are treated as (fast) bit-operations.

Perf-wise this could be nuint, so all the division and modulo operations are treated as (fast) bit-operations.

That's interesting. We rarely use native width types except in interop contexts. @jkotas @EgorBo is it correct that in hot code of this type, it can have perf benefit for simple storage of counters and offsets?

Afair, JIT knows that Span.Length (and Array.Length as well) are never negative so it doesn't need extra efforts from the C# side 🙂 e.g.:

int Foo(Span<int> s) => s.Length % 4;

; Method Foo(System.Span`1[int]):int:this mov eax, dword ptr [rdx+08H] and eax, 3 ret ; Total bytes of code: 7

(some of the changes landed in .NET 8.0 so might be not visible on sharplab yet)

Right -- I'm not aware of patterns where nuint gives better code.

JIT knows that Span.Length (and Array.Length as well) are never negative

Thanks for the reminder -- I'll forget sometimes about the new JIT-goodness.

We rarely use native width types except in interop contexts

Quite a lot of vectorized code uses the native width types, because memory operations don't need (sign-) extending moves then (e.g. Unsafe.Add(ref ptr, intVariable) vs. Unsafe.Add(ref ptr, nuintVariable) -- https://godbolt.org/z/v8E31Wdsx).

Would Math.DivRem be more efficient here? Especially once it becomes an intrinsic.

Would Math.DivRem be more efficient here? Especially once it becomes an intrinsic.

I doubt it, since the JIT optimizes length % 2 != 0 to (length & 1) != 0.

That comment wasn't just about the single length check, but also the division done a few lines later. DivRem in theory combines the two operations.

the division done a few lines later

The JIT can optimize (uint)length / 2 to (uint)length >> 1. On X86, DivRem would likely be implemented using a div instruction, which is an relatively expensive operation.

danmoseley · 2023-05-21T19:57:23Z

went to see whether we have perf coverage and we may be missing the "from hex string" case here. if so might be good to add
https://github.com/dotnet/performance/blob/a39cc8085f5738f1bce11209ec06979003d84ce1/src/benchmarks/micro/libraries/System.Runtime.Extensions/Perf.Convert.cs#L89

hrrrrustic · 2023-05-21T21:18:41Z

@danmoseley Also created a PR with benchmark
This branch looks stable (previous CI passed with some unrelated errors on IOS), but there is a comment from Stephen #78472 (comment)

ghost · 2023-05-24T21:45:33Z

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

Issue Details

Close #78472

Author:	hrrrrustic
Assignees:	-
Labels:	`area-System.Runtime`, `community-contribution`
Milestone:	-

xtqqczze · 2023-05-25T15:32:03Z

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

+                return true;
+            }
+
+            int requiredByteCount = length / 2;


Suggested change

int requiredByteCount = length / 2;

int requiredByteCount = (int)((uint)length / 2);

The JIT doesn't recognize length as never negative otherwise.

@EgorBo Should we open a JIT issue?

hrrrrustic · 2023-07-30T23:37:36Z

I'll try. Pushing updates that will probably fail CI since I didn't check them yet, just a checkpoint before sleep 😆

tannergooding · 2023-09-19T20:31:10Z

@hrrrrustic, we should be able to get this reviewed now. Could you merge in upstream so we can rerun CI?

danmoseley · 2023-09-25T04:34:31Z

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

+            Span<char> buffer = stackalloc char[loopCount * 2];
+            for (int i = 1; i < loopCount; i++)
+            {
+                byte[] data = Security.Cryptography.RandomNumberGenerator.GetBytes(i);


We usually use a seeded Random so the tests are reproducible.

I agree with Dan, but since the existing tests in this type were already using Security.Cryptography.RandomNumberGenerator it's acceptable to keep it.

adamsitnik

Overall it LGTM, I found some things that need to be adjusted before merging, but I've simply provided suggestions for all of them and I am about to apply them right now.

@hrrrrustic thank you for your contribution!

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

adamsitnik · 2023-10-31T07:36:13Z

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

+            if (destination.Length < quotient)
+            {
+                source = source.Slice(0, destination.Length * 2);
+                quotient = destination.Length;


subjective nit: using a variable called quotient to store both "quotient" and "bytesWritten" is confusing to me, we could simply assign bytesWritten to destination.Length here

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

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

adamsitnik · 2023-10-31T08:10:27Z

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

+            Span<char> buffer = stackalloc char[loopCount * 2];
+            for (int i = 1; i < loopCount; i++)
+            {
+                byte[] data = Security.Cryptography.RandomNumberGenerator.GetBytes(i);


I agree with Dan, but since the existing tests in this type were already using Security.Cryptography.RandomNumberGenerator it's acceptable to keep it.

adamsitnik

LGTM, thank you @hrrrrustic !

hrrrrustic added 7 commits May 19, 2023 01:22

Add implementation

3a0c31e

Add tests

cc13dca

Update conditions

eecaed0

Add goto

b8cb6dd

Some fixes

772208f

Some fixes

7333ca3

Add comments

f23df24

ghost added needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners community-contribution Indicates that the PR has been added by a community member labels May 21, 2023

danmoseley reviewed May 21, 2023

View reviewed changes

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

hrrrrustic added 3 commits May 21, 2023 21:10

Few more tests

70c9321

Update ref file

f80a8d3

twiced -> twice

9fd5c99

MihaZupan reviewed May 21, 2023

View reviewed changes

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

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

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

hrrrrustic added 3 commits May 21, 2023 22:13

Fix null test

d063d4f

try fix roundtrip test

986c8fa

fix tabs & naming

c644e59

gfoidl reviewed May 21, 2023

View reviewed changes

var -> int

794eff8

hrrrrustic marked this pull request as ready for review May 21, 2023 20:50

hrrrrustic mentioned this pull request May 21, 2023

Add Convert.FromHex benchmark dotnet/performance#3032

Merged

This was referenced May 21, 2023

Failed USB connection via port 54050, error 61, in tvOS arm64 Release AllSubsets_Mono #82637

Closed

Build ios-arm64 Release AllSubsets_Mono failures dotnet/arcade#13625

Closed

kiminuo mentioned this pull request May 23, 2023

[API Proposal]: Convert.TryFromHexString #78472

Closed

danmoseley added area-System.Runtime and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels May 24, 2023

xtqqczze reviewed May 25, 2023

View reviewed changes

hrrrrustic added 4 commits July 31, 2023 01:56

fix slicing

80431ab

More tests

38d018f

update xml comments

948616f

Fill values on invalid data result

af59a7f

ghost removed the needs-author-action An issue or pull request that requires more info or actions from the author. label Jul 30, 2023

This was referenced Jul 31, 2023

Tracking issue for CI build timeouts #76454

Closed

[6.0] System.Random test failure: NextInt_AllValuesAreWithinSpecifiedRange #83421

Open

hrrrrustic added 2 commits July 31, 2023 20:00

Fix tests?

dbfe893

Try update loop

30afc43

build-analysis bot mentioned this pull request Jul 31, 2023

Timeout in System.Net.Quic.Functional.Tests #86019

Closed

hrrrrustic marked this pull request as ready for review August 2, 2023 18:28

hrrrrustic requested review from adamsitnik and danmoseley August 3, 2023 21:53

Merge remote-tracking branch 'origin/main' into issue-78472

010c836

determ1ne mentioned this pull request Sep 22, 2023

Convert.ToHexStringLower: lower variant for Convert.ToHexString #92483

Merged

hrrrrustic added 3 commits September 24, 2023 19:38

try fix target buffer size error

f532064

Remove Vectorized copy-paste

9afd0cb

fix naming

e7439d4

danmoseley reviewed Sep 25, 2023

View reviewed changes

adamsitnik self-assigned this Oct 30, 2023

adamsitnik reviewed Oct 31, 2023

View reviewed changes

Apply suggestions from code review

f247ed0

adamsitnik approved these changes Oct 31, 2023

View reviewed changes

build-analysis bot mentioned this pull request Oct 31, 2023

CI error: System.Net.Quic.QuicException: The connection timed out from inactivity #91757

Closed

adamsitnik added this to the 9.0.0 milestone Oct 31, 2023

adamsitnik merged commit 0a837d3 into dotnet:main Oct 31, 2023

cincuranet mentioned this pull request Nov 7, 2023

Regressions in System.Perf_Convert #94477

Closed

ghost locked as resolved and limited conversation to collaborators Nov 30, 2023

	int requiredByteCount = length / 2;
	int requiredByteCount = (int)((uint)length / 2);

Conversation

hrrrrustic commented May 21, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

EgorBo May 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gfoidl May 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BrennanConroy May 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danmoseley commented May 21, 2023

Uh oh!

hrrrrustic commented May 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented May 24, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hrrrrustic commented Jul 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tannergooding commented Sep 19, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adamsitnik left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adamsitnik left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

EgorBo May 21, 2023 •

edited

Loading

gfoidl May 22, 2023 •

edited

Loading

BrennanConroy May 25, 2023 •

edited

Loading

hrrrrustic commented May 21, 2023 •

edited

Loading

hrrrrustic commented Jul 30, 2023 •

edited

Loading