-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Reduce fixed overhead of some Utf8Parser.TryParse methods #33507
Reduce fixed overhead of some Utf8Parser.TryParse methods #33507
Conversation
{ | ||
if (source[0] != begin) | ||
if (source[0] != (byte)ends) |
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 JIT could produce better codegen for this pattern. See #33504.
How does this relate to #32843? Can similar tweaks be made to the corresponding char-based APIs (e.g. int.Parse)? |
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.
another really nice improvement @GrabYourPitchforks !
LGTM, thanks for adding the comment that explains the bit shifting logic
src/libraries/System.Private.CoreLib/src/System/Buffers/Text/Utf8Parser/ParserHelpers.cs
Outdated
Show resolved
Hide resolved
@stephentoub That PR was primarily concerned with improving the workhorse routine. From that investigation came the idea of improving the entry point. This PR focuses just on the fixed overhead of the entry points while leaving the workhorse routines largely unchanged. I haven't tried applying these optimizations to the char overloads yet because their entry points generally look different than these entry points. But I imagine once we get back to the PR that optimizes the workhorse routines the optimizations there will be generally applicable to both char and byte. |
src/libraries/System.Private.CoreLib/src/System/Buffers/Text/Utf8Parser/Utf8Parser.Guid.cs
Show resolved
Hide resolved
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.
Changes LGTM. Had a couple questions about the codegen
...aries/System.Private.CoreLib/src/System/Buffers/Text/Utf8Parser/Utf8Parser.Integer.Signed.cs
Show resolved
Hide resolved
src/libraries/System.Private.CoreLib/src/System/Buffers/Text/Utf8Parser/Utf8Parser.Guid.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Private.CoreLib/src/System/Buffers/Text/Utf8Parser/Utf8Parser.Guid.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Private.CoreLib/src/System/Buffers/Text/Utf8Parser/Utf8Parser.Guid.cs
Outdated
Show resolved
Hide resolved
...ies/System.Private.CoreLib/src/System/Buffers/Text/Utf8Parser/Utf8Parser.Integer.Unsigned.cs
Show resolved
Hide resolved
src/libraries/System.Private.CoreLib/src/System/Buffers/Text/Utf8Parser/ParserHelpers.cs
Show resolved
Hide resolved
CI failures seem to be unrelated infrastructure failures. |
This builds atop the work that Andy did as part of #33004. There are no behavioral changes here - only slight refactorings that produce better codegen at the entry points to some of the
Utf8Parser.TryParse
methods. In particular, this does not include many of the optimizations discussed at #32843.Integral types
For the
TryParse
methods that work with integral types, reflowing the logic in this fashion moves the "default" behavior into a fast-path and reduces the total amount of logic in the switch statement. We're now able to tail-call into the workhorse methods without performing any register shuffling or stack spillingTryParse(..., out bool, ...)
Minor improvement here to avoid the range check when dereferencing
source[4]
. There's also less register shuffling in the error path. Avoiding this shuffling doesn't impact the success case, but I saw it as low-hanging fruit since it reduces the overall method codegen size by a little bit.TryParse(..., out Guid, ...)
Similar to the integral types, the switch statement has been restructured to have the common case go through a fast path. Additionally, by changing the signature of the
TryParseGuidCore
method, we can avoid the stack spillage that would normally result on Win64 from passing so many parameters to the workhorse routine.