-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
BigInteger parsing optimizations #47842
Conversation
Tagging subscribers to this area: @tannergooding, @pgovind Issue DetailsImproves performance of BigInteger parsing from decimal and hex strings (both speed and memory allocations), according to the benchmark that I have included. The difference is particularly significant for large decimal strings, given that the current implementation allocates two BigInteger instances for each input digit. Benchmark codeusing System.Collections.Generic;
using System.Globalization;
using System.Linq;
using BenchmarkDotNet.Attributes;
public class BigIntegerParseBenchmarks
{
public IEnumerable<object> NumberStrings =>
new string[]
{
"123",
int.MinValue.ToString(),
string.Concat(Enumerable.Repeat("1234567890", 20)),
string.Concat(Enumerable.Repeat("654719003", 57)),
};
public IEnumerable<object> NumberStringsHex =>
new string[]
{
"12A",
int.MinValue.ToString("X"),
string.Concat(Enumerable.Repeat("1234567890abcdefffff", 10)),
string.Concat(Enumerable.Repeat("18a473f5d", 57)),
};
[Benchmark]
[ArgumentsSource(nameof(NumberStrings))]
public BigInteger Parse(string numberString) => BigInteger.Parse(numberString);
[Benchmark]
[ArgumentsSource(nameof(NumberStringsHex))]
public BigInteger ParseHex(string numberString) => BigInteger.Parse(numberString, NumberStyles.HexNumber);
} Benchmark results (on master branch)BenchmarkDotNet=v0.12.1.1466-nightly, OS=Windows 10.0.18363.1316 (1909/November2019Update/19H2)
Intel Core i7-8550U CPU 1.80GHz (Kaby Lake R), 1 CPU, 8 logical and 4 physical cores
.NET SDK=5.0.102
[Host] : .NET 6.0.0 (6.0.21.10203), X64 RyuJIT
Job-NSGODO : .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
Results after changesBenchmarkDotNet=v0.12.1.1466-nightly, OS=Windows 10.0.18363.1316 (1909/November2019Update/19H2)
Intel Core i7-8550U CPU 1.80GHz (Kaby Lake R), 1 CPU, 8 logical and 4 physical cores
.NET SDK=5.0.102
[Host] : .NET 6.0.0 (6.0.21.10203), X64 RyuJIT
Job-WMUKDU : .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
|
src/libraries/System.Runtime.Numerics/src/System/Numerics/BigNumber.cs
Outdated
Show resolved
Hide resolved
Grepped under libraries/*/src/**cs for interest and I see object - 2 there are no uses I see for int or uint. |
I wonder if there is something we can do here to promote better sharing. Particularly when using |
|
src/libraries/System.Runtime.Numerics/src/System/Numerics/BigInteger.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
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/BigInteger.cs
Outdated
Show resolved
Hide resolved
3041198
to
2383bd3
Compare
2383bd3
to
7b5eb62
Compare
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
{ | ||
char c = number.digits[i]; |
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.
@stephentoub, is this an opportunity for an analyzer? Indexing into StringBuilder
can be deceptively slow due to its internal chunking.
isNegative = true; | ||
foreach (ReadOnlyMemory<char> digitsChunkMem in number.digits.GetChunks()) | ||
{ | ||
ReadOnlySpan<char> chunkDigits = digitsChunkMem.Span; |
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.
Likewise, I wonder if we should have a (internal for now) CurrentSpan
property that is slightly more efficient.
We don't really need to return a ReadOnlyMemory
when we only want to extract the underlying Span<T>
, particularly when the ROM<T>
is just being constructed over a backing array.
It's, in practice, no more unsafe than say CollectionsMarshal.AsSpan
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
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.
LGTM. I think it can be merged provided @stephentoub has no additional feedback
Worth an issue do you think @tannergooding |
/azp run runtime |
Azure Pipelines successfully started running 1 pipeline(s). |
Improves performance of BigInteger parsing from decimal and hex strings (both speed and memory allocations), according to the benchmark that I have included. The difference is particularly significant for large decimal strings, given that the current implementation allocates two BigInteger instances for each input digit.
Benchmark code
Benchmark results (on master branch)
Results after changes