Refactor MSM to reduce code duplication #1031
Conversation
waamm
requested review from
Pratyush,
weikengchen and
z-tech
and removed request for
a team
waamm
force-pushed
the
refactor-msm
branch
3 times, most recently
from
750b1f5 to
4daf283
Compare
|
|
||
| fn msm_u8<V: VariableBaseMSM>(mut bases: &[V::MulBase], mut scalars: &[u8]) -> V { | ||
| /// Generic MSM for small integer scalars. | ||
| fn msm_small_int_generic<V, S>(mut bases: &[V::MulBase], mut scalars: &[S]) -> V |
There was a problem hiding this comment.
Why not call this one msm_u64? No point adding the word generic unless you're doing something strange.
There was a problem hiding this comment.
I guess I'd prefer msm_small_int since this function is barely related to u64, but up to you?
There was a problem hiding this comment.
I've no real opinion aside from generic seemed redundent.
There was a problem hiding this comment.
Ok I've renamed it (and replaced the invocations inside msm_signed). In hindsight, I'm not entirely sure this PR is timely as the code feels quite WIP: when I try a recent arkworks commit in my codebase instead of version 0.5.0, some functions using MSMs run a bit faster while others are much slower...
There was a problem hiding this comment.
Hm I’d be interested to learn more about the cases where you experienced a slowdown; do you have a link?
There was a problem hiding this comment.
Hm what machine are you running on? I wonder if some change happened to the underlying field arithmetic
There was a problem hiding this comment.
I’ll try on an M1 later today or tomorrow and report back.
In the meantime, maybe this PR should be approved or closed and this discussion continued elsewhere?
There was a problem hiding this comment.
Similar results on an M1. (Again, this is for switching from 0.5.0 to your branch, where I had to modify std on this line to ark_std to get it to compile, executing env RAYON_NUM_THREADS=2 cargo bench --bench range_proof.)
3 participants
Description
msm_u8,msm_u16,msm_32, andmsm_u64have been replaced with a single generic functionmsm_small_int.msm_binaryregarding its range.msm_serial(it doesn't seem to have performance implications).Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.
Pendingsection inCHANGELOG.md(not needed for small refactor)Files changedin the GitHub PR explorer