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

Add edge case handling for batch_add #169

Merged
merged 8 commits into from
Jul 25, 2024
Merged

Add edge case handling for batch_add #169

merged 8 commits into from
Jul 25, 2024

Conversation

davidnevadoc
Copy link
Contributor

@davidnevadoc davidnevadoc commented Jul 3, 2024

This PR adds a new version of batch_add that accounts for edge cases. (Closes #153)
The old function is now batch_add_nonexceptional ( as suggested #130 (comment))
The new version is batch_add_exceptional

The decrease in performance after the change is minimal:

msm/multicore/10        time:   [5.0757 ms 5.0857 ms 5.0968 ms]
                        change: [+0.9692% +1.2777% +1.5974%] (p = 0.00 < 0.05)
                        Change within noise threshold.
msm/multicore/12        time:   [14.928 ms 14.983 ms 15.013 ms]
                        change: [-0.4586% -0.0956% +0.2811%] (p = 0.63 > 0.05)
                        No change in performance detected.
msm/multicore/14        time:   [23.467 ms 23.691 ms 23.821 ms]
                        change: [+0.2488% +1.2795% +2.4484%] (p = 0.04 < 0.05)
                        Change within noise threshold.
msm/multicore/16        time:   [72.387 ms 73.117 ms 74.886 ms]
                        change: [+0.8051% +2.4199% +5.0474%] (p = 0.04 < 0.05)
                        Change within noise threshold.

The msm functions have been renamed:
multiexp_serial -> serial_multiexp
best_multiexp -> parallel_multiexp
best_multiexp_indepenedent_points -> best_multiexp

I still think this names are quite confusing, so suggestions are welcome. ( Maybe cyclone_ instead of best_ and msm instead of multiexp).

The old batch_add has been kept but is now unsused. We can either remove it, or add another msm or argument to best_multiexp that enables its use.

@kilic kilic self-requested a review July 6, 2024 14:56
@davidnevadoc davidnevadoc marked this pull request as ready for review July 11, 2024 18:23
@davidnevadoc davidnevadoc requested a review from ed255 July 11, 2024 18:25
@davidnevadoc
Copy link
Contributor Author

Wait for #168 to be merged.

Copy link

@guorong009 guorong009 left a comment

Choose a reason for hiding this comment

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

Overall, I think the PR looks good.

I have 2 comments.

  • Is there any use case of batch_add_nonexceptional?
    I think we can get rid of this function if not used.
    It will reduce the code.
    Also, we can keep the original name - batch_add, just add the missing logic.
  • How about multiexp_serial/multiexp_parallel, instead of serial_multiexp/parallel_multiexp?
    I just believe that multiexp_* is more meaningful than *_multiexp.

Copy link

@guorong009 guorong009 left a comment

Choose a reason for hiding this comment

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

Regarding the naming, I think we should use msm, instead of multiexp.
Since we are handling the EC points and scalars.

src/msm.rs Outdated Show resolved Hide resolved
@davidnevadoc
Copy link
Contributor Author

I'm still on the fence about the naming.

How about multiexp_serial/multiexp_parallel, instead of serial_multiexp/parallel_multiexp?
I don't care about the order really, but best_multiexp breaks the pattern then...

We could have msm_parallel, msm_serial and msm_best. WDYT? @guorong009

@guorong009
Copy link

I'm still on the fence about the naming.

How about multiexp_serial/multiexp_parallel, instead of serial_multiexp/parallel_multiexp?
I don't care about the order really, but best_multiexp breaks the pattern then...

We could have msm_parallel, msm_serial and msm_best. WDYT? @guorong009

Yes, I believe they are better.

Copy link
Collaborator

@kilic kilic left a comment

Choose a reason for hiding this comment

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

lgtm :)

@davidnevadoc davidnevadoc added this pull request to the merge queue Jul 25, 2024
Merged via the queue into main with commit 44e142f Jul 25, 2024
12 checks passed
jonathanpwang pushed a commit to axiom-crypto/halo2curves that referenced this pull request Aug 13, 2024
)

* feat: add edge case handling for batch_add

* feat: handle edge cases in msm + rename functions

* chore: generate test points in parallel

* chore: remove redundant cfg

* refactor: rename msm functions

* chore: remove batch_add w/o edge case handling

* fix: clippy

* fix: leftover comment
jonathanpwang pushed a commit to axiom-crypto/halo2curves that referenced this pull request Aug 13, 2024
)

* feat: add edge case handling for batch_add

* feat: handle edge cases in msm + rename functions

* chore: generate test points in parallel

* chore: remove redundant cfg

* refactor: rename msm functions

* chore: remove batch_add w/o edge case handling

* fix: clippy

* fix: leftover comment
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cyclone MSM panic
3 participants