[resubmit] BigInteger parsing optimization for large decimal string #55121
Conversation
Co-authored-by: Stephen Toub <stoub@microsoft.com>
|
Tagging subscribers to this area: @dotnet/area-system-numerics Issue Details
Current I created branch from #47842 as it looks like #47842 will be merged shortly. benchmark result
Previous methodBenchmarkDotNet=v0.12.1.1528-nightly, OS=Windows 10.0.19042.928 (20H2/October2020Update)
Intel Core i7-7500U CPU 2.70GHz (Kaby Lake), 1 CPU, 4 logical and 2 physical cores
.NET SDK=6.0.100-preview.3.21202.5
[Host] : .NET 6.0.0 (6.0.21.20104), X64 RyuJIT
Job-CHCQPG : .NET 6.0.0 (42.42.42.42424), X64 RyuJIT
PowerPlanMode=00000000-0000-0000-0000-000000000000 Arguments=/p:DebugType=portable Toolchain=CoreRun
IterationTime=250.0000 ms MaxIterationCount=20 MinIterationCount=15
WarmupCount=1
Implemented methodBenchmarkDotNet=v0.12.1.1528-nightly, OS=Windows 10.0.19042.928 (20H2/October2020Update)
Intel Core i7-7500U CPU 2.70GHz (Kaby Lake), 1 CPU, 4 logical and 2 physical cores
.NET SDK=6.0.100-preview.3.21202.5
[Host] : .NET 6.0.0 (6.0.21.20104), X64 RyuJIT
Job-YASIZJ : .NET 6.0.0 (42.42.42.42424), X64 RyuJIT
PowerPlanMode=00000000-0000-0000-0000-000000000000 Arguments=/p:DebugType=portable Toolchain=CoreRun
IterationTime=250.0000 ms MaxIterationCount=20 MinIterationCount=15
WarmupCount=1
|
terrajobst
added
the
community-contribution
|
@tannergooding This PR is assigned to you for follow-up/decision before the RC1 snap. |
|
Thanks for the contribution here @key-moon. Due to it being so late in the cycle, this is going to miss out on the .NET 6 release and we'll instead be able to merge it after the RC1 snap and we begin accepting changes for .NET 7 (ETA: 3-4 weeks) |
|
@key-moon I wanted to let you know we're still working on some .NET 6.0 RC2 items, so we're a bit delayed on reviewing this. It'll likely be a couple more weeks. Thanks for your contributions and patience! |
| try | ||
| { | ||
| foreach (ReadOnlyMemory<char> digitsChunk in number.digits.GetChunks()) | ||
| if (number.digits.Length <= s_naiveThreshold) | ||
| { |
There was a problem hiding this comment.
Given how big each path is, I would if it would be better to break them into 2 helper methods. Basically leaving:
if (number.digits.Length <= s_naiveThreshold)
{
AlgorithmA(...);
}
else
{
AlgorithmB(...);
}-- The method is getting pretty big, which means the JIT might give up on optimizing it otherwise (haven't confirmed if it actually does).
There was a problem hiding this comment.
Sorry for late replying. I think this is worth doing in terms of improving readability. I will implement it as soon as possible.
There was a problem hiding this comment.
I have implemented and benchmarked. As a result, there is no significant difference in speed and memory. However, readability has definitely been improved.
Benchmark result
BenchmarkDotNet=v0.13.1.1611-nightly, OS=Windows 10.0.22000
11th Gen Intel Core i7-1165G7 2.80GHz, 1 CPU, 8 logical and 4 physical cores
.NET SDK=6.0.100-rc.1.21463.6
[Host] : .NET 5.0.9 (5.0.921.35908), X64 RyuJIT
Job-VEBSQX : .NET 6.0.0 (42.42.42.42424), X64 RyuJIT
Job-HLAVXS : .NET 6.0.0 (42.42.42.42424), X64 RyuJIT
PowerPlanMode=00000000-0000-0000-0000-000000000000 Arguments=/p:DebugType=portable,-bl:benchmarkdotnet.binlog IterationTime=250.0000 ms
MaxIterationCount=20 MinIterationCount=15 WarmupCount=1
| Method | Job | Toolchain | numberString | Mean | Error | StdDev | Median | Min | Max | Ratio | RatioSD | Gen 0 | Gen 1 | Gen 2 | Allocated |
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Parse | Job-VEBSQX | \artifacts-a9942c\bin\testhost\net6.0-windows-Release-x64\shared\Microsoft.NETCore.App\6.0.0\corerun.exe | 12345678901(...)01234567890 [50000] | 10.50 ms | 0.350 ms | 0.389 ms | 10.56 ms | 9.902 ms | 11.42 ms | 1.00 | 0.00 | 312.5000 | 93.7500 | - | 2 MB |
| Parse | Job-HLAVXS | \artifacts-460664\bin\testhost\net6.0-windows-Release-x64\shared\Microsoft.NETCore.App\6.0.0\corerun.exe | 12345678901(...)01234567890 [50000] | 10.29 ms | 0.384 ms | 0.427 ms | 10.34 ms | 9.563 ms | 11.13 ms | 0.98 | 0.03 | 312.5000 | 93.7500 | - | 2 MB |
| Parse | Job-VEBSQX | \artifacts-a9942c\bin\testhost\net6.0-windows-Release-x64\shared\Microsoft.NETCore.App\6.0.0\corerun.exe | 12345678901(...)01234567890 [100000] | 36.77 ms | 1.531 ms | 1.702 ms | 36.58 ms | 33.684 ms | 40.74 ms | 1.00 | 0.00 | 1125.0000 | 250.0000 | - | 7 MB |
| Parse | Job-HLAVXS | \artifacts-460664\bin\testhost\net6.0-windows-Release-x64\shared\Microsoft.NETCore.App\6.0.0\corerun.exe | 12345678901(...)01234567890 [100000] | 36.64 ms | 1.445 ms | 1.606 ms | 36.23 ms | 33.401 ms | 39.95 ms | 1.00 | 0.07 | 1166.6667 | 166.6667 | - | 7 MB |
| Parse | Job-VEBSQX | \artifacts-a9942c\bin\testhost\net6.0-windows-Release-x64\shared\Microsoft.NETCore.App\6.0.0\corerun.exe | 12345678901(...)01234567890 [150000] | 41.08 ms | 1.350 ms | 1.555 ms | 40.75 ms | 38.486 ms | 44.25 ms | 1.00 | 0.00 | 1000.0000 | 500.0000 | - | 7 MB |
| Parse | Job-HLAVXS | \artifacts-460664\bin\testhost\net6.0-windows-Release-x64\shared\Microsoft.NETCore.App\6.0.0\corerun.exe | 12345678901(...)01234567890 [150000] | 40.97 ms | 0.742 ms | 0.762 ms | 41.26 ms | 39.155 ms | 42.20 ms | 1.01 | 0.04 | 1000.0000 | 500.0000 | - | 7 MB |
| Parse | Job-VEBSQX | \artifacts-a9942c\bin\testhost\net6.0-windows-Release-x64\shared\Microsoft.NETCore.App\6.0.0\corerun.exe | 12345678901(...)01234567890 [200000] | 129.14 ms | 3.434 ms | 3.527 ms | 129.89 ms | 120.135 ms | 133.89 ms | 1.00 | 0.00 | 4500.0000 | 500.0000 | - | 28 MB |
| Parse | Job-HLAVXS | \artifacts-460664\bin\testhost\net6.0-windows-Release-x64\shared\Microsoft.NETCore.App\6.0.0\corerun.exe | 12345678901(...)01234567890 [200000] | 130.05 ms | 2.571 ms | 2.279 ms | 130.45 ms | 125.832 ms | 134.43 ms | 1.01 | 0.04 | 4500.0000 | 500.0000 | - | 28 MB |
| Parse | Job-VEBSQX | \artifacts-a9942c\bin\testhost\net6.0-windows-Release-x64\shared\Microsoft.NETCore.App\6.0.0\corerun.exe | 12345678901(...)01234567890 [250000] | 160.84 ms | 8.920 ms | 9.544 ms | 158.21 ms | 146.879 ms | 179.63 ms | 1.00 | 0.00 | 5500.0000 | 500.0000 | - | 34 MB |
| Parse | Job-HLAVXS | \artifacts-460664\bin\testhost\net6.0-windows-Release-x64\shared\Microsoft.NETCore.App\6.0.0\corerun.exe | 12345678901(...)01234567890 [250000] | 162.85 ms | 7.046 ms | 8.114 ms | 161.09 ms | 145.608 ms | 178.37 ms | 1.01 | 0.08 | 5500.0000 | 500.0000 | - | 34 MB |
| Parse | Job-VEBSQX | \artifacts-a9942c\bin\testhost\net6.0-windows-Release-x64\shared\Microsoft.NETCore.App\6.0.0\corerun.exe | 12345678901(...)01234567890 [300000] | 132.28 ms | 3.731 ms | 3.992 ms | 131.79 ms | 122.929 ms | 137.93 ms | 1.00 | 0.00 | 3500.0000 | 1500.0000 | 1000.0000 | 24 MB |
| Parse | Job-HLAVXS | \artifacts-460664\bin\testhost\net6.0-windows-Release-x64\shared\Microsoft.NETCore.App\6.0.0\corerun.exe | 12345678901(...)01234567890 [300000] | 134.72 ms | 5.300 ms | 6.103 ms | 133.63 ms | 123.651 ms | 147.27 ms | 1.03 | 0.06 | 3500.0000 | 1500.0000 | 1000.0000 | 24 MB |
| Parse | Job-VEBSQX | \artifacts-a9942c\bin\testhost\net6.0-windows-Release-x64\shared\Microsoft.NETCore.App\6.0.0\corerun.exe | 12345678901(...)01234567890 [350000] | 356.66 ms | 20.078 ms | 21.484 ms | 351.60 ms | 321.365 ms | 406.99 ms | 1.00 | 0.00 | 14000.0000 | 12000.0000 | 11000.0000 | 71 MB |
| Parse | Job-HLAVXS | \artifacts-460664\bin\testhost\net6.0-windows-Release-x64\shared\Microsoft.NETCore.App\6.0.0\corerun.exe | 12345678901(...)01234567890 [350000] | 354.54 ms | 17.497 ms | 19.448 ms | 348.05 ms | 323.012 ms | 396.16 ms | 0.99 | 0.07 | 15000.0000 | 13000.0000 | 12000.0000 | 71 MB |
| Parse | Job-VEBSQX | \artifacts-a9942c\bin\testhost\net6.0-windows-Release-x64\shared\Microsoft.NETCore.App\6.0.0\corerun.exe | 12345678901(...)01234567890 [400000] | 524.19 ms | 47.271 ms | 54.437 ms | 510.63 ms | 461.465 ms | 640.54 ms | 1.00 | 0.00 | 17000.0000 | 3000.0000 | 2000.0000 | 107 MB |
| Parse | Job-HLAVXS | \artifacts-460664\bin\testhost\net6.0-windows-Release-x64\shared\Microsoft.NETCore.App\6.0.0\corerun.exe | 12345678901(...)01234567890 [400000] | 553.00 ms | 91.333 ms | 105.179 ms | 487.60 ms | 444.111 ms | 710.82 ms | 1.06 | 0.21 | 17000.0000 | 3000.0000 | 2000.0000 | 107 MB |
| Parse | Job-VEBSQX | \artifacts-a9942c\bin\testhost\net6.0-windows-Release-x64\shared\Microsoft.NETCore.App\6.0.0\corerun.exe | 12345678901(...)01234567890 [450000] | 627.40 ms | 91.780 ms | 105.695 ms | 570.04 ms | 526.129 ms | 800.22 ms | 1.00 | 0.00 | 24000.0000 | 1000.0000 | - | 149 MB |
| Parse | Job-HLAVXS | \artifacts-460664\bin\testhost\net6.0-windows-Release-x64\shared\Microsoft.NETCore.App\6.0.0\corerun.exe | 12345678901(...)01234567890 [450000] | 554.17 ms | 49.428 ms | 56.921 ms | 542.68 ms | 472.918 ms | 657.23 ms | 0.90 | 0.14 | 24000.0000 | 1000.0000 | - | 149 MB |
src/libraries/System.Runtime.Numerics/src/System/Numerics/BigNumber.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Runtime.Numerics/src/System/Numerics/BigNumber.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Runtime.Numerics/src/System/Numerics/BigNumber.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Runtime.Numerics/src/System/Numerics/BigNumber.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Runtime.Numerics/src/System/Numerics/BigNumber.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Runtime.Numerics/src/System/Numerics/BigNumber.cs
Outdated
Show resolved
Hide resolved
|
The overall logic LGTM. I left a couple nits/comments about adding comments and possibly minor refactorings to help with readability when this inevitably is looked at again in the future. I likewise called out a peculiar scenario around handling fractional digits which I don't believe is possible (happy to be proven wrong here). It would be great if this could get a second pair of eyes on it (CC. @pgovind, @stephentoub, @bartonjs since you've reviewed or worked on BigInteger semi-recently). |
src/libraries/System.Runtime.Numerics/src/System/Numerics/BigNumber.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Runtime.Numerics/tests/BigInteger/BigNumberTools.cs
Show resolved
Hide resolved
| multiplier[0] = TenPowMaxPartial; | ||
|
|
||
| // This loop is executed ceil(log_2(bufferSize)) times. | ||
| while (true) |
There was a problem hiding this comment.
If this loop is executed ceil(log_2(bufferSize)), why do you not use a for loop? I think these are better optimized by the JIT.
There was a problem hiding this comment.
Please benchmark before/after to make sure that this is really the case.
src/libraries/System.Runtime.Numerics/src/System/Numerics/BigNumber.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Runtime.Numerics/tests/BigInteger/BigNumberTools.cs
Show resolved
Hide resolved
src/libraries/System.Runtime.Numerics/tests/BigInteger/BigNumberTools.cs
Show resolved
Hide resolved
| } | ||
| } | ||
|
|
||
| private static volatile TypeInfo s_lazyInternalNumber; |
There was a problem hiding this comment.
Can you add a comment, which explains why this is volatile.
There was a problem hiding this comment.
This commits explains the reason. Actually, I didn't understand why volatile is required.
dotnet/corefx@8918578
|
@key-moon are you still working on this? No particular rush, just wanted to ensure that was the case since many people took a break over the holidays. There are still several pending comments above that would need to be resolved. |
tannergooding
added
the
needs-author-action
|
Yes, I’m still working on this PR. Sorry for the late response. I'll resolve these conversations as soon as possible. |
ghost
removed
the
needs-author-action
tannergooding
added
the
needs-author-action
ghost
removed
the
needs-author-action
|
This has a merge conflict that needs to be resolved. I also resolved a few comments around styling concerns where the code in question isn't new, rather just being moved around. While I do think that resolving those styling inconsistencies would be nice, since its not new code I think it can happen in an independent PR. |
|
Closing and reopening to try and get helix to pick up the changes and allow CI to actually run. |
|
This PR looks generally good, I'd just like to get CI passing before merging. Merging with |
|
Build failures are unrelated |
|
Thanks @key-moon for your work on this perf improvement! |
…otnet#55121) * implement divide-and-conquer method for parsing digits * fix argument order in Assert when x equals to 0 * Apply format fix Co-authored-by: Stephen Toub <stoub@microsoft.com> * add test for non-naive algorithm * add description for naiveThreshold * fix trivial part * add check for boundary condition * add assertions and descriptions * change variable name * remove inappropreate use of var * to use ArrayPool<int>.Shared.Rent for newBuffer allocation * move both algorithms to separate methods * add and fix comments * trivial fix Co-authored-by: Stephen Toub <stoub@microsoft.com>
ghost
locked as resolved and limited conversation to collaborators
Current
BigNumer.NumberToBigIntegermethod is implemented using naive algorithm. It runs inΘ(N^2)time where N is number of digits. I implemented faster method known as divide-and-conquer algorithm. It runsΘ(N (log(N))^2). Since this algorithms running time has large constant factor, naive method is faster when N is small. So This method is only apply when N is large enough. (specifically, use divide-and-conquer method when N is more than 20000.)I created branch from #47842 as it looks like #47842 will be merged shortly.
benchmark result
Previous method
Implemented method