Skip to content

Add edge case handling for batch_add#169

Merged
davidnevadoc merged 8 commits into
mainfrom
nev@batch_add_edge
Jul 25, 2024
Merged

Add edge case handling for batch_add#169
davidnevadoc merged 8 commits into
mainfrom
nev@batch_add_edge

Conversation

@davidnevadoc

@davidnevadoc davidnevadoc commented Jul 3, 2024

Copy link
Copy Markdown
Collaborator

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
Copy Markdown
Collaborator Author

Wait for #168 to be merged.

@guorong009 guorong009 left a comment

Copy link
Copy Markdown
Contributor

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.

@guorong009 guorong009 left a comment

Copy link
Copy Markdown
Contributor

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.

Comment thread src/msm.rs Outdated
@davidnevadoc

Copy link
Copy Markdown
Collaborator 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
Copy Markdown
Contributor

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.

@kilic kilic left a comment

Copy link
Copy Markdown
Contributor

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
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