Optimize HexConverter.FromChar
#81146
Conversation
xtqqczze
commented
Jan 25, 2023
•
xtqqczze
commented
- Eliminate bounds checks
- Clarify conditions
ghost
added
the
community-contribution
|
I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label. |
| } | ||
|
|
||
| [MethodImpl(MethodImplOptions.AggressiveInlining)] | ||
| public static int FromUpperChar(int c) | ||
| { | ||
| return c > 71 ? 0xFF : CharToHexLookup[c]; |
There was a problem hiding this comment.
This looks like an off-by-one error in the original code since 'F' is 70.
There was a problem hiding this comment.
It's not an actual error because c being 70 will return 0xFF anyway (from the lookup)
xtqqczze
changed the title
HexConverter.FromChar
|
Tagging subscribers to this area: @dotnet/area-system-runtime Issue Details
|
| @@ -275,13 +275,13 @@ public static bool TryDecodeFromUtf16(ReadOnlySpan<char> chars, Span<byte> bytes | |||
| [MethodImpl(MethodImplOptions.AggressiveInlining)] | |||
| public static int FromChar(int c) | |||
| { | |||
| return c >= CharToHexLookup.Length ? 0xFF : CharToHexLookup[c]; | |||
| return (uint)c >= 'f' + 1 ? 0xFF : CharToHexLookup[c]; | |||
There was a problem hiding this comment.
(uint)c > 'f' ? 0xFF : CharToHexLookup[c];
There was a problem hiding this comment.
This would make sense if the size of CharToHexLookup could be reduced. Otherwise, what is the benefit of this optimization?
There was a problem hiding this comment.
The JIT seems to depend on the GE operand to perform the elimination (#35348).
There was a problem hiding this comment.
This would make sense if the size of CharToHexLookup could be reduced. Otherwise, what is the benefit of this optimization?
We already do this for FromUpperChar. One benefit is code size is reduced, see sharplab
I'm also assuming it's beneficial to avoid an unnecessary load.
There was a problem hiding this comment.
I mean CharToHexLookup size could be reduced to 128.
There was a problem hiding this comment.
I mean CharToHexLookup size could be reduced to 128.
It doesn't look so - there are places where it's accessed without cheking bounds using a byte indexer.
There was a problem hiding this comment.
I have seen a whole series of PRs trying to reduce the size of this library. So the question is whether it's important enough to add one command but reduce the size.
|
Worth noting that it probably doesn't remove bound checks in cases where input is known to be non-zero as those are already handled as is, e.g. consider this:
|
|
Draft Pull Request was automatically closed for 30 days of inactivity. Please let us know if you'd like to reopen it. |
ghost
locked as resolved and limited conversation to collaborators
