Skip to content
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

[DRAFT] BigInteger.Divide perf optimization #89211

Closed
wants to merge 2 commits into from

Conversation

adamsitnik
Copy link
Member

I implemented the suggestion provided by @Grevor in #41495 (comment)

The removal of bound checks has no visible effect on the perf (+-1%, within the range of error), but removing the branch does impact the perf a lot:

- if (leftElement < digit)
-     ++carry;
- leftElement = unchecked(leftElement - digit);
+ ulong newDigit = unchecked((ulong)leftElement - digit);
+ carry += (newDigit >> 32) & 0x1; // This is the same as if (leftElement < digit) ++carry
+ leftElement = unchecked((uint)newDigit);

The problem is that for some cases it improves the perf, while for other it regresses. @tannergooding do you have any idea why this could be the case here?

BenchmarkDotNet=v0.13.2.2052-nightly, OS=Windows 11 (10.0.22621.1992)
AMD Ryzen Threadripper PRO 3945WX 12-Cores, 1 CPU, 24 logical and 12 physical cores
.NET SDK=8.0.100-preview.4.23259.14
Method Job arguments Mean Ratio
Divide PR 1024,512 bits 547.1 ns 1.37
Divide main 1024,512 bits 399.2 ns 1.00
Divide PR 4096,1024 bits 5,352.4 ns 1.35
Divide main 4096,1024 bits 3,958.7 ns 1.00
Divide PR 65536,32768 bits 1,424,991.9 ns 0.33
Divide main 65536,32768 bits 4,278,792.9 ns 1.00
Divide PR 8192,2048 bits 18,954.8 ns 0.47
Divide main 8192,2048 bits 40,343.7 ns 1.00

I am not sure whether I'll have free cycles to dig in more into that. I am just opening the PR to start a discussion.

cc @speshuric

@EgorBo
Copy link
Member

EgorBo commented Jul 19, 2023

The removal of bound checks has no visible effect on the perf (+-1%, within the range of error),

Maybe then revert that part? There is an on-going effort to reduce number of unsafe hacks (e.g. bound checks) in sake of better safety.

@adamsitnik adamsitnik closed this Sep 22, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Oct 22, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants