-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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.Divide #96895
Optimize BigInteger.Divide #96895
Conversation
Tagging subscribers to this area: @dotnet/area-system-numerics Issue DetailsThe current Burnikel-Ziegler Algorithm is used in other languages.
|
27070b7
to
dd63906
Compare
There's a few different BigInteger PRs open right now, this is on my list to get to but I want to try and get some of the other ones completed/merged first |
dd63906
to
5079b54
Compare
1ac62fc
to
c72c0b6
Compare
Will this optimized Divide also result in a faster GreatestCommonDivisor method? I'm on the verge of rewriting a lot of C# code to Rust, only because of the BigInteger performance. |
@lskyum Just curious, what do you use big integer for? |
@danmoseley I'm using two BigInteger's to represent a "BigFraction", when performing various geometric calculations. |
@lskyum The division also affects a GCD method.
[GlobalSetup]
public void Setup()
{
o1 = OrigBigInteger.Pow(3, N) * OrigBigInteger.Pow(5, N);
o2 = OrigBigInteger.Pow(3, N) * OrigBigInteger.Pow(23, N);
m1 = MyBigInteger.Pow(3, N) * MyBigInteger.Pow(5, N);
m2 = MyBigInteger.Pow(3, N) * MyBigInteger.Pow(23, N);
} Environment: https://github.com/kzrnm/BigInteger/tree/main/Benchmark Benchmark code
using BenchmarkDotNet.Attributes;
using BenchmarkDotNet.Configs;
using BenchmarkDotNet.Jobs;
using BenchmarkDotNet.Reports;
using BenchmarkDotNet.Running;
using BenchmarkDotNet.Toolchains.CsProj;
using MyBigInteger = Kzrnm.Numerics.BigInteger;
using OrigBigInteger = System.Numerics.BigInteger;
public class BenchmarkConfig : ManualConfig
{
static void Main(string[] args)
{
#if DEBUG
BenchmarkSwitcher.FromAssembly(typeof(BenchmarkConfig).Assembly).Run(args, new DebugInProcessConfig());
#else
_ = BenchmarkRunner.Run(typeof(BenchmarkConfig).Assembly);
#endif
}
public BenchmarkConfig()
{
AddExporter(BenchmarkDotNet.Exporters.MarkdownExporter.GitHub);
AddJob(Job.ShortRun.WithToolchain(CsProjCoreToolchain.NetCoreApp80));
SummaryStyle = SummaryStyle.Default
.WithRatioStyle(BenchmarkDotNet.Columns.RatioStyle.Value)
;
}
}
[Config(typeof(BenchmarkConfig))]
[GroupBenchmarksBy(BenchmarkLogicalGroupRule.ByParams)]
public class BigIntegerGcd
{
[Params(10000, 100000)]
public int N;
public OrigBigInteger o1, o2;
public MyBigInteger m1, m2;
[GlobalSetup]
public void Setup()
{
o1 = OrigBigInteger.Pow(3, N) * OrigBigInteger.Pow(5, N);
o2 = OrigBigInteger.Pow(3, N) * OrigBigInteger.Pow(23, N);
m1 = MyBigInteger.Pow(3, N) * MyBigInteger.Pow(5, N);
m2 = MyBigInteger.Pow(3, N) * MyBigInteger.Pow(23, N);
}
[Benchmark(Baseline = true)]
public OrigBigInteger Orig() => OrigBigInteger.GreatestCommonDivisor(o1, o2);
[Benchmark]
public MyBigInteger New() => MyBigInteger.GreatestCommonDivisor(m1, m2);
} |
@kzrnm That looks really awesome! Thanks for making this possible. |
Do we have enough coverage in dotnet/performance to protect this change? |
The 65536 bits correspond to a uint array of length 4096, which would cover this change. |
|
||
return ActualLength(bits.Slice(0, modulus.Length)); | ||
} | ||
return bits.Length; | ||
} | ||
|
||
[Conditional("DEBUG")] | ||
public static void DummyForDebug(Span<uint> bits) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs a better name, like InitializeForDebug
or InitializeAsCleanMemoryForDebug
, as this is the constant used for "clean memory" in MSVC
Alternatively it shouldn't exist at all as we typically don't do such debug vs release differences today. Any such support should rather be centralized across the .NET libraries or done automatically by the runtime.
src/libraries/System.Runtime.Numerics/src/System/Numerics/BigIntegerCalculator.DivRem.cs
Show resolved
Hide resolved
src/libraries/System.Runtime.Numerics/src/System/Numerics/BigIntegerCalculator.DivRem.cs
Show resolved
Hide resolved
src/libraries/System.Runtime.Numerics/src/System/Numerics/BigIntegerCalculator.DivRem.cs
Show resolved
Hide resolved
src/libraries/System.Runtime.Numerics/src/System/Numerics/BigIntegerCalculator.DivRem.cs
Show resolved
Hide resolved
src/libraries/System.Runtime.Numerics/src/System/Numerics/BigIntegerCalculator.DivRem.cs
Show resolved
Hide resolved
src/libraries/System.Runtime.Numerics/src/System/Numerics/BigIntegerCalculator.DivRem.cs
Show resolved
Hide resolved
Debug.Assert((uint)sigmaSmall <= 32); | ||
int carryShift = 32 - sigmaSmall; | ||
uint carry = 0; | ||
for (int i = rt.Length - 1; i >= 0; i--) | ||
{ | ||
remainder[i] = rt[i] >> sigmaSmall | carry; | ||
carry = rt[i] << carryShift; | ||
} | ||
Debug.Assert(carry == 0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: the code here and other places is very squashed together. This makes it harder to read
We should allow for inserting newlines to help break apart the considerations and help improve the legibility. This applies to several cases in this PR, but this one I found personally troublesome to parse. A better formatting that helps me is:
Debug.Assert((uint)sigmaSmall <= 32);
int carryShift = 32 - sigmaSmall;
uint carry = 0;
for (int i = rt.Length - 1; i >= 0; i--)
{
remainder[i] = rt[i] >> sigmaSmall | carry;
carry = rt[i] << carryShift;
}
Debug.Assert(carry == 0);
Potentially inserting explanatory comments for why things like the asserts are expected to hold true, as that helps future readers who are not as familiar with the algorithm or its details
Code looks correct to me, but there are some necessary formatting changes (required by the coding-style guidelines) before this can be merged. I've also suggested a couple renames of helper methods (required) and opportunities to improve readability of the code (not required, but appreciated). Once those are resolved, this can get merged. Sorry again for the delay in getting it reviewed, it ended up being too late in the cycle for .NET 9 and work in other areas kept deprioritizing the ability to get the review finished. |
@tannergooding Thank you for your review! I've made formatting changes. |
* main: JIT: Set PGO data inconsistent when flow disappears in cast expansion (dotnet#112147) [H/3] Fix handling H3_NO_ERROR (dotnet#112125) Change some workflows using `pull_request` to use `pull_request_target` instead (dotnet#112161) Annotate ConfiguredCancelableAsyncEnumerable T with allows ref struct and update extensions (dotnet#111953) Delete copy of performance pipelines in previous location (dotnet#112113) Optimize BigInteger.Divide (dotnet#96895) Use current STJ in HostModel and remove unnecessary audit suppressions (dotnet#109852) JIT: Unify handling of InstParam argument during inlining (dotnet#112119) Remove unneeded DiagnosticSource content (dotnet#112116) Improve compare-and-branch sequences produced by Emitter (dotnet#111797) Jit: Conditional Escape Analysis and Cloning (dotnet#111473) Re-enable HKDF-SHA3 on Azure Linux Remove fstream usage from corehost (dotnet#111859)
The current$O(n^2)$ where n is the number of digits. The implementation of the pull request is implemented in Burnikel and Ziegler's Fast Recursive Division algorithm. It runs in $O(n^{\log 3} + n \log n)$ .
BigInteger.Divide
, which is implemented in the "grammar-school" algorithm, runs inBurnikel-Ziegler Algorithm is used in other languages.