-
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
Spannified internals of BigInteger #35565
Conversation
Tagging subscribers to this area: @tannergooding |
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.
You can get rid of some Unsafe.Add
usage by letting the JIT do the work and have the safety back.
(Didn't comment on all places, please do one pass of clean-up.)
Plus some nits.
src/libraries/System.Runtime.Numerics/src/System/Numerics/BigInteger.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Runtime.Numerics/src/System/Numerics/BigInteger.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Runtime.Numerics/src/System/Numerics/BigInteger.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Runtime.Numerics/src/System/Numerics/BigInteger.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Runtime.Numerics/src/System/Numerics/BigIntegerCalculator.AddSub.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Runtime.Numerics/src/System/Numerics/BigIntegerCalculator.BitsBuffer.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Runtime.Numerics/src/System/Numerics/BigIntegerCalculator.DivRem.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Runtime.Numerics/src/System/Numerics/BigIntegerCalculator.FastReducer.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Runtime.Numerics/src/System/Numerics/BigIntegerCalculator.FastReducer.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Runtime.Numerics/src/System/Numerics/BigIntegerCalculator.FastReducer.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Runtime.Numerics/src/System/Numerics/BigIntegerCalculator.AddSub.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Runtime.Numerics/src/System/Numerics/BigIntegerCalculator.AddSub.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Runtime.Numerics/src/System/Numerics/BigIntegerCalculator.AddSub.cs
Outdated
Show resolved
Hide resolved
@gfoidl , could you please take a look at the next iteration (commit a1e5006). I tried to reduce memory allocation for bitwise operations. But I'm not satisfied with necessity of delegate type that is introduced to reduce code duplication. Moreover, this approach leads to delegate instance allocation on every call. AFAIK, the current version of C# still doesn't support method pointers.
|
src/libraries/System.Runtime.Numerics/src/System/Numerics/BigInteger.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Runtime.Numerics/src/System/Numerics/BigInteger.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Runtime.Numerics/src/System/Numerics/BigInteger.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Runtime.Numerics/src/System/Numerics/BigInteger.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Runtime.Numerics/src/System/Numerics/BigInteger.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Runtime.Numerics/src/System/Numerics/BigIntegerCalculator.DivRem.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Runtime.Numerics/src/System/Numerics/BigIntegerCalculator.DivRem.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Runtime.Numerics/src/System/Numerics/BigIntegerCalculator.DivRem.cs
Outdated
Show resolved
Hide resolved
@GrabYourPitchforks sounds good |
@sakno I haven't used it myself, but I believe you can run this on the before and after results to get a better comparison: |
I just add this to my
where those |
Just checking in on this one since it's so old, seems like the next action is @GrabYourPitchforks offered to have another look through. And @sakno if you are interested you could try the trick above to get a single table. |
I had something come up which will make me busy for the next few days, but I can continue driving this once my schedule frees up. Should we un-milestone from 6.0 then? |
Yes, they won't take this change into 6.0. |
|
@sakno Thanks for sharing those performance numbers. We're finally down to the final stages of getting this merged! @GrabYourPitchforks, @bartonjs, @tannergooding, and I all spent time reviewing the performance results, looking over the changes again, and discussing with each other what the risks and benefits are with these changes. With such a large refactoring, I needed to ensure we're looking at this from all angles, including potential future improvements. We've concluded that the improvements you've made here do indeed put us in a better position overall. Thank you! I've asked @GrabYourPitchforks to resolve the 2 conflicts that exist right now; he'll do that later this week and push directly to the PR's branch. After that, assuming the CI shows up as green, we'll finally merge this in. These changes will then be part of .NET 7.0 Preview 1. Thank you so much for your persistence and all of your effort on this, @sakno! This is a significant contribution to .NET. |
@jeffhandley , @GrabYourPitchforks , thanks for the feedback! I can resolve the conflict by myself without any problem if you want. However, I see the usage of index expression ( |
Where did I reject it? |
@stephentoub , here is reverting PR: #57297. It was also applied to this PR.
The motivation was:
|
Right, that wasn't me rejecting their usage, that was their usage in those cases showing to regress performance. |
@sakno Thanks! We'd happily take you up on the offer to resolve the 2 conflicts. And good catch on the range expressions having |
@jeffhandley , done. |
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.
We missed one thing in earlier reviews, but otherwise commit 65485fa LGTM! 🥳
uint divHi = right[rightLength - 1]; | ||
uint divLo = rightLength > 1 ? right[rightLength - 2] : 0; | ||
uint divHi = right[right.Length - 1]; | ||
uint divLo = right.Length > 1 ? right[right.Length - 2] : 0; | ||
|
||
// We measure the leading zeros of the divisor | ||
int shift = LeadingZeros(divHi); |
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.
Cleanup opportunity we didn't spot earlier: get rid of this LeadingZeros
method and prefer BitOperations.LeadingZeroCount
instead.
@sakno We're at the final step! If you want to address #35565 (comment) (it's just deleting some dead code) as part of this PR, please feel free to do so, otherwise we can tackle it in a follow-up PR. Whatever you decide, I think we're good to merge right after we kick CI a bit. |
@GrabYourPitchforks , Oh sorry, not applicable. Also, |
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.
Thank you, @sakno! 💯
Proposed refactoring according with issue: #22609. The implementation plan consists of the following steps:
BitsBuffer
value typeFastReducer
value typeArrayPool<T>
?BMI intrinsics(moved to separated issue BigInteger performance improvements #41495)Spannified versions of internal and private static methods look pretty nice. However, I'm not sure about performance of passing span to the method. If RyuJIT uses scalar replacement then it's good news. Otherwise, maybe pass length and managed pointer to the first element as separate arguments to ensure that they passed through registers. This version was implemented in the first commit. I need advice here as well as preliminary code review because further work fully based on signatures of spannified methods.