Skip to content
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.

Sign up for GitHub

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

Optimize HexConverter.FromChar #81146

Closed
wants to merge 2 commits into from
Closed

Optimize HexConverter.FromChar #81146

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ce5873c
Eliminate FromChar bounds checks
xtqqczze Jan 25, 2023
3b3d0db
Optimize conditions
xtqqczze Jan 25, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/libraries/Common/src/System/HexConverter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

(uint)c > 'f' ? 0xFF : CharToHexLookup[c];

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would make sense if the size of CharToHexLookup could be reduced. Otherwise, what is the benefit of this optimization?

Copy link
Contributor Author

@xtqqczze xtqqczze Jan 25, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The JIT seems to depend on the GE operand to perform the elimination (#35348).

https://csharp.godbolt.org/z/Peqfx8KfM

Copy link
Contributor Author

@xtqqczze xtqqczze Jan 25, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

Line 284 in 9b76c28

return c > 71 ? 0xFF : CharToHexLookup[c];

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Filed #81160

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean CharToHexLookup size could be reduced to 128.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

[MethodImpl(MethodImplOptions.AggressiveInlining)]
public static int FromUpperChar(int c)
{
return c > 71 ? 0xFF : CharToHexLookup[c];
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like an off-by-one error in the original code since 'F' is 70.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not an actual error because c being 70 will return 0xFF anyway (from the lookup)

return (uint)c >= 'F' + 1 ? 0xFF : CharToHexLookup[c];
}

[MethodImpl(MethodImplOptions.AggressiveInlining)]
Expand Down