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: specialize the multiplication by D in twistededward packages #445

Closed
wants to merge 3 commits into from

Conversation

yelhousni
Copy link
Collaborator

Description

This PR was motivated by #442 and #441. Twisted Edwards curves are not as optimized as Weierstrass ones in gnark-crypto, since the formers are only used for tests in gnark. Anyway, this PR is an example to push the performance a bit further.

Multiplication by the constant D is used is several places:

  • Affine addition
  • projective addition
  • projective mixed addition
  • extended addition
  • Curve membership check
  • X coordinate retrieval

This PR specialize this multiplication for baby-Jubjub, Jubjub and Bandersnatch twisted Edwards curves. The same can be done for other curves but only these are actually used in projects (maybe BLS12-377 companion curve too e.g. Aleo, Penumbra?).

Type of change

  • Performance improvement

How has this been tested?

This PR doesn’t change the logic or add new border cases, so existing tests cover correct behavior.

How has this been benchmarked?

  • For baby-Jubjub, this saves in Fr: 5 doublings and 81 additions;
  • For Jubjub, this saves in Fr: 6 doublings and 79 additions;
  • For Bandersnatch, this saves in Fr: 6 doublings and 78 additions.

@yelhousni yelhousni added the perf label Sep 6, 2023
@yelhousni yelhousni added this to the v0.10.0 milestone Sep 6, 2023
@jsign
Copy link
Contributor

jsign commented Sep 6, 2023

@yelhousni, this is the bench comparison between master (old) and d818e5dc (new) for Bandersnatch:

name                    old time/op    new time/op    delta
ScalarMulExtended-16      34.2µs ± 1%   167.9µs ± 2%  +390.49%  (p=0.000 n=10+10)
ScalarMulProjective-16    34.7µs ± 2%   168.2µs ± 2%  +384.68%  (p=0.000 n=10+10)
Neg/Affine-16             2.18ns ± 2%    2.20ns ± 1%      ~     (p=0.063 n=10+10)
Neg/Projective-16         2.22ns ± 1%    2.22ns ± 2%      ~     (p=0.676 n=9+10)
Neg/Extended-16           4.87ns ± 0%    4.84ns ± 2%      ~     (p=0.436 n=9+9)
MixedAdd/Projective-16     219ns ± 0%    2024ns ± 1%  +822.35%  (p=0.000 n=8+10)
MixedAdd/Extended-16       224ns ± 1%     225ns ± 0%      ~     (p=0.795 n=9+6)
Add/Affine-16             4.63µs ± 1%    6.49µs ± 2%   +40.15%  (p=0.000 n=8+10)
Add/Projective-16          237ns ± 2%    2010ns ± 1%  +748.70%  (p=0.000 n=10+9)
Add/Extended-16            200ns ± 2%    1960ns ± 2%  +882.12%  (p=0.000 n=10+8)
IsOnCurve/positive-16      106ns ± 2%     872ns ± 3%  +718.84%  (p=0.000 n=10+9)
IsOnCurve/negative-16      108ns ± 1%     853ns ± 3%  +692.34%  (p=0.000 n=8+9)

So maybe something is odd?

@yelhousni
Copy link
Collaborator Author

yelhousni commented Sep 6, 2023

So maybe something is odd?

Oh wow!! ^^ that's so weird, let me check

@yelhousni
Copy link
Collaborator Author

ok that was a silly PR, my bad :) In my head, it was a generic scalar multiplication that I replaced by a short addition chain but here the multiplication is in Fp so that's really nonsense :) Closing this.

@yelhousni yelhousni closed this Sep 6, 2023
@yelhousni yelhousni deleted the perf/twistededwards/mulBy branch September 6, 2023 21:57
@yelhousni yelhousni restored the perf/twistededwards/mulBy branch September 7, 2023 16:35
@yelhousni yelhousni deleted the perf/twistededwards/mulBy branch September 7, 2023 18:37
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