Deduplicate Binary Integer parsing logic #84582
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Tagging subscribers to this area: @dotnet/area-system-numerics Issue DetailsThis utilizes generic math to deduplicate most of the Binary Integer parsing logic. Doing so simplifies the code we need to maintain and will also make it easier to onboard UTF-8 support
|
tannergooding
force-pushed
the
simplify-parse
branch
from
0e4441f
to
0a83ed3
Compare
stephentoub
reviewed
Apr 11, 2023
src/libraries/System.Private.CoreLib/src/System/Number.Parsing.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Private.CoreLib/src/System/Globalization/NumberFormatInfo.cs
Show resolved
Hide resolved
jeffhandley
added
the
NO-MERGE
|
I've marked this as
NO-MERGE
|
Perf for RyuJITPerf_Byte
Perf_Int16
Perf_Int32
Perf_Int64
Perf_SByte
Perf_UInt16
Perf_UInt32
Perf_UInt64
|
|
MonoInterpreter is fine overall. It, however, really doesn't like changes such as: public static int Parse(string s)
{
- if (s == null) ThrowHelper.ThrowArgumentNullException(ExceptionArgument.s);
- return Number.ParseInt32(s, NumberStyles.Integer, NumberFormatInfo.CurrentInfo);
+ return Parse(s, NumberStyles.Integer, provider: null);
}
public static int Parse(string s, NumberStyles style, IFormatProvider? provider)
{
- NumberFormatInfo.ValidateParseStyleInteger(style);
- if (s == null) ThrowHelper.ThrowArgumentNullException(ExceptionArgument.s);
- return Number.ParseInt32(s, style, NumberFormatInfo.GetInstance(provider));
+ ArgumentNullException.ThrowIfNull(s);
+ return Parse(s.AsSpan(), style, provider);
}
public static int Parse(ReadOnlySpan<char> s, NumberStyles style = NumberStyles.Integer, IFormatProvider? provider = null)
{
NumberFormatInfo.ValidateParseStyleInteger(style);
return Number.ParseInt32(s, style, NumberFormatInfo.GetInstance(provider));
}
Is there anything we can do here to improve Mono for such scenarios? Being able to reduce the duplicated code here makes the code a lot more readable/maintainable and is a fairly standard practice throughout the BCL and in user code. Perf for MonoInterpreterPerf_Byte
Perf_Int16
Perf_Int32
Perf_Int64
Perf_SByte
Perf_UInt16
Perf_UInt32
Perf_UInt64
|
|
Similar overall results for MonoJIT Perf for MonoJITPerf_Byte
Perf_Int16
Perf_Int32
Perf_Int64
Perf_SByte
Perf_UInt16
Perf_UInt32
Perf_UInt64
|
|
CC. @SamMonoRT for input on the MonoInterpreter/MonoJIT results above? Its about a 30ns regression in the worst case. Reintroducing the validation duplication brings it down to about a 5ns regression, but brings back about 300 lines of code and makes the code atypical to a lot of other code in the BCL and community apps. We certainly can do this, if there are concerns about the regression; but long term this seems like another common pattern that Mono would ideally handle. -- Noting that outside the args validation, we are seeing perf wins in the interpreter for some of the |
|
Thanks @tannergooding - these regressions are acceptable given the scope of the changes. Are you targeting wasm-aot runs too? |
tannergooding
removed
the
NO-MERGE
Haven't yet, let me get those numbers as well. |
|
@SamMonoRT, don't currently have a Unix machine setup so I won't be able to test until sometime next week. Trying to run on windows ends up hitting:
There are also a couple other quirks (which I tried working around), such as:
Ideally this would work and be testable on Windows as well, since that is one of the primary development environments for many users. |
stephentoub
reviewed
Apr 15, 2023
|
@SamMonoRT, I'm not able to get the wasm-aot benchmarks working in WSL following the instructions given. Seeing: |
|
jeffhandley
approved these changes
Apr 17, 2023
There was a problem hiding this comment.
Thanks, @tannergooding! I appreciate you reverting the throw helper approach and also gathering the mono perf numbers and looking at how different approaches have trade-offs.
|
Given that we've gathered perf numbers on a variety of coreclr and mono backends and the numbers looked good, and CI is green, and Tanner's tried to get wasm-aot numbers and is blocked from doing so, I think it's reasonable to merge. |
|
Will react to and handle any uncaught impact as needed. |
This was referenced Apr 19, 2023
It looks like you are missing v8 engine. I have opened dotnet/performance#2974 to mention it in the workflow doc. |
This was referenced Apr 20, 2023
Closed
|
Improvements on arm64: |
This was referenced Apr 25, 2023
Closed
Closed
Closed
This was referenced Apr 26, 2023
Closed
Closed
ghost
locked as resolved and limited conversation to collaborators
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
This utilizes generic math to deduplicate most of the Binary Integer parsing logic. Doing so simplifies the code we need to maintain and will also make it easier to onboard UTF-8 support