feat: Elliptic Curve Method (ECM) for integer factorization#174
Open
s-celles wants to merge 8 commits intoJuliaMath:mainfrom
Open
feat: Elliptic Curve Method (ECM) for integer factorization#174s-celles wants to merge 8 commits intoJuliaMath:mainfrom
s-celles wants to merge 8 commits intoJuliaMath:mainfrom
Conversation
The previous implementation was GMP-only, forcing callers to convert to BigInt even when a fixed-width type (e.g. UInt256 from BitIntegers.jl) would be much faster. Changes in src/ecm.jl: - Add _addmod / _submod / _mulmod generic helpers that use widen(T) for overflow-safe arithmetic. For BitIntegers types the widen chain stays in LLVM fixed-width integers (UInt256 → UInt512 → UInt1024), so no BigInt allocation occurs in the inner loop. - Add generic _ecm_add / _ecm_double / _ecm_scalar_mul (functional style, no mutation) that work for any T<:Integer. - Add generic ecm_factor(n::T, ...) where T<:Integer dispatching to the generic helpers. - Retain the BigInt-specific ecm_factor(n::BigInt, ...) overload using GMP in-place arithmetic (ECMBuffers) for large BigInt factorizations. The _ecm_add! / _ecm_double! / _ecm_scalar_mul! (bang variants) are BigInt-only and called exclusively from this overload. - Remove MontgomeryCurvePoint struct (unused after refactor). - Update _ecm_scalar_mul! to accept k::Integer (was k::BigInt). Changes in test/runtests.jl: - Add testset 'ECM generic integer types' covering UInt128 (widen→BigInt path) and UInt256/Int256 from BitIntegers.jl (widen→UInt512 LLVM path). Changes in Project.toml: - Add BitIntegers as a test-only dependency ([extras] + [targets]).
Use 'a >= n - b ? a - (n - b) : a + b' instead of computing a + b first, which could silently wrap around for unsigned T when n is close to typemax(T). Also eliminate redundant _addmod/_submod calls in _ecm_double by computing the sum and difference of PX, PZ once.
Extract _ecm_prime_powers(B1) and _ecm_suyama(n) shared helpers used by both the generic and BigInt-optimized ecm_factor methods. The Suyama parametrization (curve setup) is not performance-critical, so both paths now share the generic modular arithmetic version. Only the hot Montgomery ladder inner loop retains a separate BigInt in-place implementation.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #174 +/- ##
==========================================
+ Coverage 93.08% 94.58% +1.49%
==========================================
Files 2 3 +1
Lines 463 646 +183
==========================================
+ Hits 431 611 +180
- Misses 32 35 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Member
|
What are the benchmarks for the GMP optimized version vs the generic version (with BigInt)? |
Author
|
Both interesting and disappointing results @oscardssmith |
Co-authored-by: Oscar Smith <oscardssmith@gmail.com>
Member
|
With the So for numbers <2^64 the Int128 path seems very helpful. It is very much a shame that the optimized BigInt versions are so much faster. Ideally we would be able to write them in a more uniform way. |
| nextprime, nextprimes, prevprime, prevprimes, prime, prodfactors, radical, totient | ||
|
|
||
| include("factorization.jl") | ||
| include("ecm.jl") |
Member
There was a problem hiding this comment.
we need to actually include ecm_factor in addition (or instead?) of polard_factor. Ideally ecm_factor would be always better and then we could delete polard, but I don't know if we're that lucky.
Author
There was a problem hiding this comment.
Well my opinion about this is a bit different. Removing this kind of code remove the possibility for users to "experiment" different factorization methods with a given number.
See #175
Member
There was a problem hiding this comment.
fair enough. We should, however, probalby remove pollard and add ecm to the default factor algorithm
Member
|
Eventually, it would be good to have the 2 stage version of ECM, but the single stage version should already be an improvement. |
Author
|
I found some time in the evening to look at stage 2 implementation. |
Member
|
I think the problem might be that you are testing ecm_factor on Int128s larger than 2^64 (which is invalid). |
Author
|
This PR is a concrete motivation for #176. function _ecm_prime_powers(B1::Int)
result = Int[]
for p in primes(B1) # allocates all primes ≤ B1
pk = p
while pk * p <= B1
pk *= p
end
push!(result, pk)
end
result
endWith function _ecm_prime_powers(B1::Int)
result = Int[]
for p in Iterators.takewhile(<=(B1), primes_from(2))
pk = p
while pk * p <= B1
pk *= p
end
push!(result, pk)
end
result
endFor typical ECM usage with large |
Member
|
IMO this is a fairly weak motivation as a B1 of 11_000 requires very little memory. I do agree that it is a good idea though. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Helps #159
Part of #173 with some BitIntegers.jl types support