Moving the Utf8Formatter and Utf8Parser into S.P.Corelib #20934
Conversation
| // results. | ||
|
|
||
| number.HasNonZeroTail = (c != '0'); | ||
| } |
There was a problem hiding this comment.
Why is this change necessary as part of moving this into corelib? Does this have any impact on throughput, seeing as though we're adding additional logic in what appears to be a few tight loops?
There was a problem hiding this comment.
CoreFX tests won't pass otherwise.
The Utf8Parser is using the NumberBuffer implementation that was already in S.P.Corelib now. The Digit buffer it contains is dynamically sized to be exactly the right size (which is enough digits to hold the largest exact string + 1 digit for rounding and + 1 digit for the null terminator). It also tracks whether any digits that were specified past the rounding digit where zero, which allows it to be correctly rounded in all scenarios.
There was a problem hiding this comment.
I'll check perf here, but I would expect some minimal impact (most of which should hopefully be handled by the branch predictor).
There was a problem hiding this comment.
@stephentoub, we have no perf tests that currently exercise this code (which is decimal, double, and float). We only have tests that cover the integer types.
- Working on getting some basic tests to cover this now...
There was a problem hiding this comment.
Really basic perf tests (just mirroring the utf16 ones) here: dotnet/corefx#33427
There was a problem hiding this comment.
@stephentoub, @GrabYourPitchforks, perf numbers look similar to the Utf16Parser/Utf16Formatter differences.
For integers (both parsing and formatting), we are the essentially the same (some faster, some slower; all look within std deviation).
For floating-point (decimal, single, and double), we are essentially the same for formatting (some faster, some slower; all look within std deviation).
For floating-point parsing, we are faster on numbers that are <= 15 digits and with exponents <= 22. For those outside the range, we are up to ~5x slower but are now returning the correct result.
- As mentioned on the Utf16Parser issue, we can improve this further for numbers <= 19 digits and with exponents <= 27 by using 80-bit extended precision multiplication/division
We can improve this for numbers <= 34 digits and with exponents <= 38 by using 128-bit floating-point multiplication/division
There was a problem hiding this comment.
Can't seem to upload the CSV files right now, GitHub is having issues doing so.
…8Formatter building
…ark the end of the buffer
|
@stephentoub, how do we want to handle the mirror? Merging this will cause the changes to mirror to CoreFX, which will cause I think the options are:
|
|
Add temporary ifdefs around the offending code |
I think there is too much code that was changed to do that, it would basically be putting an |
src/System.Private.CoreLib/shared/System/Buffers/Text/Utf8Parser/Utf8Parser.Number.cs
Outdated
Show resolved
Hide resolved
src/System.Private.CoreLib/shared/System/Buffers/Text/Utf8Parser/Utf8Parser.Number.cs
Outdated
Show resolved
Hide resolved
It shouldn't (famous last words) be a long delay between this being merged and a new build being produced and consumed into corefx. If ifdef'ing is too painful, you could just wait to merge the mirror PR until that's happened, at which point you can add a commit with the appropriate fix-ups to the mirror PR. Reasonable? |
That is basically the first option, I listed 😄
|
|
Ah, then yeah, if ifdef'ing doesn't work, that seems reasonable. |
| // | ||
| public static bool TryFormatThrowFormatException(out int bytesWritten) | ||
| { | ||
| bytesWritten = 0; |
There was a problem hiding this comment.
Nit: swap this line with the line immediately below. Otherwise the write has an observable side effect that the JIT cannot elide.
(Don't let this block checkin.)
| // | ||
| public static bool TryParseThrowFormatException(out int bytesConsumed) | ||
| { | ||
| bytesConsumed = 0; |
There was a problem hiding this comment.
Same nit as earlier.
…clr#20934) * Moving the Utf8Formatter and Utf8Parser into S.P.Corelib * Doing some minimal cleanup to lineup types and get the Utf8Parser/Utf8Formatter building * Updating the Utf8 Float Parser to have different buffers for Single vs Double * Fixing the Utf8Parser to track trailing zero digits and to properly mark the end of the buffer * Fixing a couple of issues in Utf8Parser.Number Commit migrated from dotnet/coreclr@80c0a0e
As per #20873 (comment), I split the PR into multiple.
This is the third PR and moves the Utf8 Formatter and Parser into S.P.Corelib.