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

perf(ecc): faster affine Add #510

Merged
merged 2 commits into from
Jun 12, 2024
Merged

Conversation

yelhousni
Copy link
Collaborator

@yelhousni yelhousni commented Jun 12, 2024

Description

Actually, we can optimize affine addition more than #509 with the assumption that both Z1 and Z2 are 1 in Jacobian coordinates. This PR uses these EFD formulas. Also #509 modifies the second input in the Sub method which this PR fixes.

Type of change

  • New feature (non-breaking change which adds functionality)

How has this been tested?

same tests as #509

How has this been benchmarked?

e.g. BLS12-381 G1 addition on a z1d.large AWS machine:

goos: linux
goarch: amd64
pkg: github.com/consensys/gnark-crypto/ecc/bls12-381
cpu: Intel(R) Xeon(R) Platinum 8151 CPU @ 3.40GHz
BenchmarkG1AffineAdd
BenchmarkG1AffineAdd-2   	  232734	      5135 ns/op
PASS

This is a -3.56% saving compared to #509

benchmark                  old ns/op     new ns/op     delta
BenchmarkG1AffineAdd-2     5135          4952          -3.56%

Checklist:

  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • I did not modify files generated from templates
  • golangci-lint does not output errors locally
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

@yelhousni yelhousni added the perf label Jun 12, 2024
@yelhousni yelhousni added this to the v0.10.0 milestone Jun 12, 2024
@yelhousni yelhousni requested a review from gbotrel June 12, 2024 11:42
@yelhousni yelhousni self-assigned this Jun 12, 2024
Copy link
Collaborator

@gbotrel gbotrel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looks correct and match the formulas, however, it is not tested (i.e. G1Affine.Add is not called in any test files, only in fr/lookup and kzg ).

Can you add some basic tests / property based tests now that we have a different code path than Jacobian?

(general addition, doubling, adding of point at infinity, and addition resulting in point at infinity)

@yelhousni
Copy link
Collaborator Author

yelhousni commented Jun 12, 2024

Code looks correct and match the formulas, however, it is not tested (i.e. G1Affine.Add is not called in any test files, only in fr/lookup and kzg ).

Can you add some basic tests / property based tests now that we have a different code path than Jacobian?

(general addition, doubling, adding of point at infinity, and addition resulting in point at infinity)

I think in TestG*AffineOps we call affine Add, Sub, Double and ScalarMul here for example:

properties.Property("[BLS12-381] [2]G = double(G) + G - G", prop.ForAll(

Also this test

properties.Property("[BLS12-381] Add should call double when adding the same point", prop.ForAll(

checks the edge case where inputs are equal in Add but I do agree that tests involving the point (0,0) are missing. Will add.

@gbotrel gbotrel self-requested a review June 12, 2024 15:10
@yelhousni yelhousni merged commit 0823e43 into master Jun 12, 2024
7 checks passed
@yelhousni yelhousni deleted the perf/faster-affine-arithmetic branch June 12, 2024 22:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants