-
Notifications
You must be signed in to change notification settings - Fork 497
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
MSM optimizations #608
MSM optimizations #608
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK with question.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK with suggestions for how to avoid the memory overhead in the map.
…ests. Signed-off-by: Daira Hopwood <daira@jacaranda.org>
Reduce memory overhead of MSM
bases.extend( | ||
self.other | ||
.iter() | ||
.map(|(x, (_, y))| C::from_xy(*x, *y).unwrap()), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See ebfull#1 (comment) for performance analysis of doing this vs storing x-coordinate -> (scalar, point).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK. I verified that the orchard
tests (including the pinned proof) pass with this change.
self.other | ||
.entry(*x) | ||
.and_modify(|(our_scalar, our_y)| { | ||
if our_y == y { | ||
*our_scalar += *scalar; | ||
} else { | ||
assert!(*our_y == -*y); | ||
*our_scalar -= *scalar; | ||
} | ||
}) | ||
.or_insert((*scalar, *y)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Non-blocking: this could be de-duplicated from add_msm
and append_term
with a helper method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I couldn't quite get that to work, so deferred it for a later cleanup.
`BatchVerifier` now manages the entire batch verification process. Individual proofs are verified on a threadpool, and the resulting MSMs are then batch-checked as before. The addition of parallelism here couples with #608 to make parallelism less fine-grained and reduce the overhead of multi-threading.
parallelize
calls from inside the MSM when operating on scalars, which has enormous overhead even for relatively large circuits.MSM
to deduplicate group elements #606)