Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Unify int to hexadecimal char conversions #1273
Unify int to hexadecimal char conversions #1273
Changes from 14 commits
1dbff64
d3f6eb9
22b1e8a
463ec5d
07666d8
0236295
168b5c9
8f89e1d
1eda24e
2e2b28a
d91d965
5b0f61a
af72883
ddd4b2e
2d11acb
e9e623e
5001cbd
aea9539
b8e9287
7c14dce
0f84e3b
7b08687
a2963c2
1116938
9a9f789
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suspect
should be a bit faster if it's used in hot paths
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems dangerous. Are all callers limited in how large
bytes
is?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see an assert at https://github.com/dotnet/runtime/pull/1273/files#diff-63d8e37549a2478236a87c352cdd1024L1132 that's been removed here. If we're sticking with the stackalloc, we should put that assert back.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added the assert (not sure how useful that's for legacy builds)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The
bytes.Length <= 16
assert is not guaranteed to be true and this stackalloc could be unsafe. For example,PhysicalAddress
takes an unboundedbyte
array from the user and we pass it here in it'sToString()
. That will result in stackoverflow. For lengths > 16 (or some reasonable small constant value, say 128), we should consider doing arraypool.rent or maybe just allocate.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does not matter for this case as the assembly is not built in affected configurations but I changed it anyway as it could show up in System.Security.Cryptography.Pkcs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be a little bit faster. But I don't know if it's worth in contrast to the delegate invocation in
string.Create
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where did we land on this implementation (are we not going for the ROSpan version)? What was the reason for not going with what @tannergooding suggested here (single branch): #1273 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tanner version is not producing expected output and ROSpan version needs codegen support first (the code can be updated once that lands)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Look like its because the subtraction was off; it needs to subtract
'9' + 1
. The following passes the unit tests (with the logic all being constant folded):There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you measure this and find it to be faster than:
? (Especially once the JIT properly removes the bounds check on this, if it doesn't already.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Or using the same tricks/match employed by ToCharsBuffer above.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@stephentoub
It doesn't today but I have a fix for JIT: EgorBo@bcc095e
so it will remove them for things like
array[i & c]
andarray[i % c]
ifc
is less thanarray.Length
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't as these are convenience methods rarely called more than a few times. The Span versions are intended for perf sensitive code.
Also, introducing global memory blob in each assembly which uses this code seems to me contra-productive size-wise.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here are the micro-benchmarks for 3 different cases. The sources for them at https://gist.github.com/marek-safar/b26e2bfc9a60a2bd348b3f36d3cb8288
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Certainly. I'm doing a build for this now and should be able to test sometime after lunch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From my Samsung Galaxy Book 2:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the
ConditionVersion
in this case the one with the 2 branches or the one with your suggestion (and only 1 branch)?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Its thr code from @marek-safar's gist, with no changes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How does the single branch condition perform on
Samsung Galaxy Book 2
in comparison to the rest? If it is on-par/better than ROSVersion, then that helps answer which approach we should take (granted, with @EgorBo's changes, the ROSVersion could get better).