-
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
Optimize BigInteger.Parse #97589
Optimize BigInteger.Parse #97589
Conversation
Tagging subscribers to this area: @dotnet/area-system-numerics Issue Details
Benchmarkusing BenchmarkDotNet.Attributes;
using BenchmarkDotNet.Running;
using System.Numerics;
using System.Runtime.InteropServices;
BenchmarkSwitcher.FromAssembly(typeof(Tests).Assembly).Run(args);
[DisassemblyDiagnoser]
public class Tests
{
[Params(1000, 10000, 100000, 1000000)]
public int N;
string s1, s2, s3;
[GlobalSetup]
public void Setup()
{
s1 = "1" + new string('0', N);
s2 = "1" + new string('0', N - 1) + "1";
s3 = new string('9', N - 1);
}
[Benchmark]
public BigInteger PowerOfTen() => BigInteger.Parse(s1);
[Benchmark(Baseline = true)]
public BigInteger PowerOfTenPlusOne() => BigInteger.Parse(s2);
[Benchmark]
public BigInteger PowerOfTenMinusOne() => BigInteger.Parse(s3);
}
|
0e0fa7f
to
7cfc562
Compare
I updated the benchmark target to the latest commit. |
Thanks! Slowly getting through stuff here and just want to ensure no conflicts/accurate numbers/etc as they go through. |
Looks like there are some related CI failures that need to get resolved. |
Tests fail when |
894bbcc
to
e2200c2
Compare
I was able to reproduce the error with the following code. Number.s_naiveThreshold = 0;
BigInteger.Parse("99000000000"); |
e2200c2
to
96f4562
Compare
@huoyaoyuan, if you have some time I'd appreciate if you could give this a once over as well, given you've also been touching/updating the parsing logic. I'd ideally like to get #95402 done first and I know there's another PR out, but you recently changed that one back to |
I've done the code update and measuring the performance. I'd expect to make it ready this weekend. |
dfde2c5
to
d69b10a
Compare
1da8f28
to
edc6a57
Compare
3daff74
to
9f5f44a
Compare
# Conflicts: # src/libraries/System.Runtime.Numerics/src/System/Numerics/BigIntegerCalculator.SquMul.cs # src/libraries/System.Runtime.Numerics/tests/BigInteger/multiply.cs
d785168
to
456bf8e
Compare
f92dff4
to
29747f2
Compare
b3b8e56
to
f01a779
Compare
@kzrnm, sorry for the delay on this. Could you share updated numbers, just so we can confirm nothing has regressed given all the other work that's been done? After that, I think we can get this merged. |
@tannergooding I ran benchmarks on the latest branch.
|
@tannergooding Regressions: Mono related regressions:
|
Just noting on the regressions above that many of them are for very small inputs, like Rather, we want to ensure that |
Optimize Multiplier
BigInteger.Parse
treatsThe leading
For example, the array represention of$10^{9\times2^8} = 10^{2304}$ has 957 items, of which the leading 288 items are 0.
Parse powers of ten
BigInteger.Parse("1" + new string('0', n))
is too slow. It runs inThis is because trailing zeros are always calculated naively.
I fixed the behavior.
Benchmark