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 all 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.
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.
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).