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

edwards: avoid inversions in Add in extended points #442

Merged
merged 2 commits into from Sep 6, 2023

Conversation

jsign
Copy link
Contributor

@jsign jsign commented Sep 4, 2023

Description

This PR proposes solving a performance problem while doing Add in Edwards curves.

The TL;DR of the problem is that this operation does an equality checking, which involves a very slow inversion.

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?

name                    old time/op    new time/op    delta
MixedAdd/Extended-16       221ns ± 2%     220ns ± 2%      ~     (p=0.393 n=10+10)
Add/Extended-16           3.70µs ± 1%    0.20µs ± 2%   -94.52%  (p=0.000 n=10+10)

name                    old alloc/op   new alloc/op   delta
MixedAdd/Extended-16       0.00B          0.00B           ~     (all equal)
Add/Extended-16            0.00B          0.00B           ~     (all equal)

name                    old allocs/op  new allocs/op  delta
MixedAdd/Extended-16        0.00           0.00           ~     (all equal)
Add/Extended-16             0.00           0.00           ~     (all equal)

@jsign jsign changed the title edwards: avoid inversions in Add and MixedAdd in extended points edwards: avoid inversions in Add in extended points Sep 4, 2023
@jsign jsign force-pushed the jsign-edwards-extended branch 2 times, most recently from 68e033f to 3a36ad5 Compare September 4, 2023 15:59
@yelhousni yelhousni self-requested a review September 4, 2023 19:59
@yelhousni yelhousni self-assigned this Sep 4, 2023
@yelhousni yelhousni added the perf label Sep 4, 2023
Copy link
Collaborator

@yelhousni yelhousni left a comment

Choose a reason for hiding this comment

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

Good catch! Add should implement the strongly unified addition and not the dedicated one + the heavy inverse-based equality check. Maybe we can add AddDedicated (without the equality check) if we think there is guaranteed use of it (e.g. MSM), although it saves just a mul by constant d.

Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com>
Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com>
@jsign jsign marked this pull request as ready for review September 5, 2023 15:05
@jsign
Copy link
Contributor Author

jsign commented Sep 5, 2023

@yelhousni, @gbotrel, rebased.

Copy link
Collaborator

@yelhousni yelhousni left a comment

Choose a reason for hiding this comment

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

👍

@yelhousni yelhousni merged commit 49815a2 into Consensys:master Sep 6, 2023
6 checks passed
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.

None yet

2 participants