-
Notifications
You must be signed in to change notification settings - Fork 222
perf: optimize Eisenstein arithmetic #706
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
Conversation
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.
Pull Request Overview
This PR optimizes Eisenstein integer arithmetic by switching to inline big.Int storage, introducing Karatsuba-based multiplication, and adding new division routines (Quo, MulByConjugate) and round-nearest logic.
Key changes:
- Redesigned
ComplexNumberto store temporaries and prevent unintended copies. - Added
MulByConjugate,roundNearest,Quo, and reworkedQuoRemandHalfGCDfor performance. - Updated tests and benchmarks to cover the new methods and signatures.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| field/eisenstein/eisenstein.go | Refactored ComplexNumber, implemented Karatsuba mul, new division and rounding methods, and adjusted HalfGCD. |
| field/eisenstein/eisenstein_test.go | Added property tests for MulByConjugate, updated Norm and QuoRem tests, and expanded benchmarks. |
Comments suppressed due to low confidence (1)
field/eisenstein/eisenstein_test.go:122
- The new property in
TestEisensteinArithmeticis never executed because there's noproperties.TestingRuncall at the end of that test function. Addproperties.TestingRun(t, gopter.ConsoleReporter(false))to ensure the property runs.
properties.Property("Mul(Conjugate) & MulByConjugate should output the same result", prop.ForAll(
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.
Bug: Nil Pointer Dereference in Complex Number Benchmarks
The new benchmark functions BenchmarkMul, BenchmarkNorm, and BenchmarkQuoRem attempt to use uninitialized *ComplexNumber pointers from the benchRes array (e.g., benchRes[0], benchRes[1]). Since benchRes elements are nil by default, these operations result in nil pointer dereference panics when methods are called or fields are accessed.
field/eisenstein/eisenstein_test.go#L394-L405
gnark-crypto/field/eisenstein/eisenstein_test.go
Lines 394 to 405 in 0dcd1ad
| for i := 0; i < b.N; i++ { | |
| benchRes[0].Mul(&a, &c) | |
| } | |
| } | |
| func BenchmarkNorm(b *testing.B) { | |
| a0, _ := new(big.Int).SetString("121538263010334165887337363056149355411", 10) | |
| a1, _ := new(big.Int).SetString("249054933928109647438301795139995905723", 10) | |
| a := ComplexNumber{A0: *a0, A1: *a1} | |
| b.ResetTimer() | |
| for i := 0; i < b.N; i++ { | |
| a.Norm(&benchRes[0].A0) |
field/eisenstein/eisenstein_test.go#L417-L418
gnark-crypto/field/eisenstein/eisenstein_test.go
Lines 417 to 418 in 0dcd1ad
| for i := 0; i < b.N; i++ { | |
| benchRes[0].QuoRem(&a, &c, benchRes[1]) |
Bug: Method Modifies Input Parameter Violating Expectations
The QuoRem method incorrectly modifies its input parameter x by using x.t1 as a temporary variable for norm calculations. This violates the expectation that input parameters remain unchanged. Specifically, when the receiver z and parameter x are the same object, bestQ0 (aliased to z.t1) is overwritten by candR.Norm(&x.t1) before the comparison, leading to incorrect quotient selection in the Euclidean division's lattice search.
field/eisenstein/eisenstein.go#L265-L270
gnark-crypto/field/eisenstein/eisenstein.go
Lines 265 to 270 in 0dcd1ad
| if candR.Norm(&x.t1).Cmp(bestNorm) < 0 { | |
| bestQ0.Set(&z.A0) | |
| bestQ1.Set(&z.A1) | |
| r.Set(&candR) | |
| bestNorm.Set(&x.t1) |
field/eisenstein/eisenstein.go#L240-L241
gnark-crypto/field/eisenstein/eisenstein.go
Lines 240 to 241 in 0dcd1ad
| // If Euclidean inequality already holds we're done. | |
| if r.Norm(&x.t1).Cmp(&y.t0) < 0 { |
Was this report helpful? Give feedback by reacting with 👍 or 👎
Description
Some optimisations for Eisenstein arithmetic. This saves -50% for Half-GCD which we use in hints for emulated scalar multiplications in gnark.
Type of change
How has this been tested?
Existing tests pass + test for new method
MulByConjugate.How has this been benchmarked?
Macbook Air M1:
Checklist:
golangci-lintdoes not output errors locally