-
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
BigInteger parsing optimization for large decimal string #51953
Conversation
Tagging subscribers to this area: @tannergooding, @pgovind Issue DetailsCurrent I created branch from #47842 because #47872 looks like merged soon. sorry for inconvenience. benchmark resultPrevious 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
|
@pgovind Could you review this when you get a chance please? |
Would you mind rebasing this PR on the latest main? I spent a little bit of time looking at the PR today and the changes from #47842 making it hard to discern this PR's changes. |
Thanks for reviewing. I rebased onto current main branch. Since it requires history modification, I force-pushed the change. Sorry for inconvenience. |
} | ||
else | ||
{ | ||
if (numberScale < 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.
Just adding a note to myself: This is the real change in the PR. The block above is just refactoring inside an if
. I'll review this PR this week. It's taking some time since I need to go through the algorithm first and then review the implementation here.
@@ -494,35 +494,242 @@ private static bool HexNumberToBigInteger(ref BigNumberBuffer number, out BigInt | |||
} | |||
} | |||
|
|||
private static int s_naiveThreshold = 20000; |
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:
private const int NaiveThreshold = 20_000;
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.
Also, it'd be helpful to add a comment about how this value was selected.
src/libraries/System.Runtime.Numerics/src/System/Numerics/BigNumber.cs
Outdated
Show resolved
Hide resolved
Co-authored-by: Stephen Toub <stoub@microsoft.com>
@key-moon and @dotnet/area-system-numerics, I just realized that this PR was still open, but the originating branch has been deleted. I'm going to close the PR, but if you want to proceed with it, we could still carry forward starting with the last commit. |
I accidentally deleted a forked repository yesterday. I want to continue from the last commit. What do I need to do? I apologize for the inconvenience caused. |
No problem; I'm sorry we let the PR sit as long as we did. I tried to see if I could recover the commit, but I'm not sure if that will work. I think the best bet here is to create a new branch and reapply the changes that are still shown in this PRs files changed tab. Since the changes look pretty well-contained, I think that'll be easier than trying to pull off some GitHub/git magic to recover the actual previous commits. From there, you can create a new PR and link to this one in the new PR description. |
Current
BigNumer.NumberToBigInteger
method 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