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

Add binary GCD #54

Merged
merged 5 commits into from
Oct 9, 2024
Merged

Add binary GCD #54

merged 5 commits into from
Oct 9, 2024

Conversation

dvdplm
Copy link
Contributor

@dvdplm dvdplm commented Oct 7, 2024

Adds binary GCD to the final step of our gcd_vartime.

The speedup is real but marginal.

Before:

	           fastest       │ slowest       │ median        │ mean          │ samples │ iters    
        Uint<16>   217.1 ns      │ 1.226 µs      │ 245 ns        │ 245.6 ns      │ 79978   │ 2559296
        Uint<32>   327.8 ns      │ 1.004 µs      │ 346.1 ns      │ 346.8 ns      │ 99701   │ 1595216
        Uint<64>   527.8 ns      │ 1.879 µs      │ 553.9 ns      │ 555.6 ns      │ 110265  │ 882120

After:

		   fastest       │ slowest       │ median        │ mean          │ samples │ iters
        Uint<16>   184.3 ns      │ 842.4 ns      │ 192.7 ns      │ 200.2 ns      │ 93287   │ 2985184
        Uint<32>   271.4 ns      │ 1.41 µs       │ 298.9 ns      │ 299.3 ns      │ 109994  │ 1759904
        Uint<64>   449.7 ns      │ 5.069 µs      │ 494.2 ns      │ 492.8 ns      │ 99620   │ 796960

The true goal here was to compare our own gcd implementation to that of crypto-bigint. If the two are comparable then we could perhaps get rid of some code. Sadly that is not the case (at all), given our second operand is a Word (aka u64) I do not think the performance difference is due to any inefficiency we can address easily. The difference is one to two orders of magnitude.

Copy link

codecov bot commented Oct 7, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.34%. Comparing base (eb89fca) to head (b7c9e33).
Report is 7 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master      #54      +/-   ##
==========================================
+ Coverage   99.33%   99.34%   +0.01%     
==========================================
  Files           9        9              
  Lines        1344     1369      +25     
==========================================
+ Hits         1335     1360      +25     
  Misses          9        9              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

src/hazmat/gcd.rs Outdated Show resolved Hide resolved
@fjarri
Copy link
Member

fjarri commented Oct 7, 2024

Not for this PR, but I wonder if we can use binary GCD to calculate the Jacobi symbol too. It should have more performance impact since it's called more often during Lucas test, although probably still not too much.

@fjarri
Copy link
Member

fjarri commented Oct 7, 2024

Also, again not for this PR, but there's a note in MillerRabin::test():

    pub fn test(&self, base: &T) -> Primality {
        // TODO: it may be faster to first check that gcd(base, candidate) == 1,
        // otherwise we can return `Composite` right away.

Worth checking out since GCD is available in crypto-bigint now.

@dvdplm
Copy link
Contributor Author

dvdplm commented Oct 9, 2024

@fjarri Is this good to go?

@fjarri fjarri merged commit e1bd96d into master Oct 9, 2024
10 checks passed
@fjarri fjarri mentioned this pull request Oct 16, 2024
@fjarri fjarri deleted the dp-faster-gcd branch October 19, 2024 00:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants