Uh oh!
There was an error while loading. Please reload this page.
Improve BigInteger operators +, - and * for trivial cases - #84733
Conversation
Move handling of trivial cases from internal methods with span arguments to to the very beginning of public operators. This avoids creating unneeded spans, checking them to empty state and some other unneeded operations. Does not affect time of processing non-trivial arguments. Discussion: #84721
ghost
commented
Apr 12, 2023
Tagging subscribers to this area: @dotnet/area-system-numerics Issue DetailsMove handling of trivial cases from internal methods with span arguments to to the very beginning of public operators. This avoids creating unneeded spans, checking them to empty state and some other unneeded operations. Does not affect time of processing non-trivial arguments. Discussion: #84721
|
Uh oh!
There was an error while loading. Please reload this page.
Address PR feedback.
danmoseley
commented
May 22, 2023
Do existing tests hit these new paths? |
speshuric
commented
May 22, 2023
Yes, existing tests cover new code paths. |
… why. To avoid that we can make an error message out of the comment. So when it fails, it's printed.
adamsitnik
left a comment
There was a problem hiding this comment.
I've merged upstream into your fork and compared the performance against main by using these benchmarks
dotnet run -c Release -f net8.0 -- --filter "*Perf_BigInteger.Add*""*Perf_BigInteger.Subtract*""*Perf_BigInteger.Multiply*" --corerun D:\projects\runtime\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\corerun.exe D:\projects\forks\biginteger\artifacts\bin\testhost\net8.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\corerun.exe --launchCount 3The performance for small inputs for + and - has improved. For larger inputs the change is within the range of error.
But the numbers show that the perf has regressed for multiplication of small inputs. It's most likely an outlier caused by something temporary like code alignment. In case it's not true, our Reporting System is going to detect this regression and we are going to revert the change for *
| Method | Job | arguments | Mean | Ratio | Allocated |
|---|---|---|---|---|---|
| Add | PR | 1024,1024 bits | 41.20 ns | 1.01 | 160 B |
| Add | main | 1024,1024 bits | 40.70 ns | 1.00 | 160 B |
| Subtract | PR | 1024,1024 bits | 42.149 ns | 0.98 | 152 B |
| Subtract | main | 1024,1024 bits | 43.579 ns | 1.00 | 152 B |
| Multiply | PR | 1024,1024 bits | 864.309 ns | 0.99 | 280 B |
| Multiply | main | 1024,1024 bits | 869.421 ns | 1.00 | 280 B |
| Add | PR | 16,16 bits | 5.786 ns | 0.60 | - |
| Add | main | 16,16 bits | 9.712 ns | 1.00 | - |
| Subtract | PR | 16,16 bits | 5.803 ns | 0.60 | - |
| Subtract | main | 16,16 bits | 9.674 ns | 1.00 | - |
| Multiply | PR | 16,16 bits | 9.762 ns | 1.17 | - |
| Multiply | main | 16,16 bits | 8.368 ns | 1.00 | - |
| Add | PR | 65536,65536 bits | 1,798.512 ns | 0.98 | 8224 B |
| Add | main | 65536,65536 bits | 1,834.566 ns | 1.00 | 8224 B |
| Subtract | PR | 65536,65536 bits | 1,781.835 ns | 0.97 | 8216 B |
| Subtract | main | 65536,65536 bits | 1,838.100 ns | 1.00 | 8216 B |
| Multiply | PR | 65536,65536 bits | 774,176.820 ns | 1.00 | 16410 B |
| Multiply | main | 65536,65536 bits | 774,391.028 ns | 1.00 | 16410 B |
@speshuric thank you for your contribution!
Uh oh!
There was an error while loading. Please reload this page.
Move handling of trivial cases from internal methods with span arguments to to the very beginning of public operators. This avoids creating unneeded spans, checking them to empty state and some other unneeded operations.
Does not affect time of processing non-trivial arguments.
Discussion: #84721