Alg Opt with GLV #4

jon-chuang · 2020-09-03T13:54:13Z

This PR is meant to resolve the code drift since the previous two PRs. It leaves the recursive batch verification code commented out. It also retains GLV. There will be a sister PR with GLV redacted. This is meant to merge atop that.

Features:

Batch Affine ops
Batch and ordinary w-NAF
GLV framework, params only for bw6. GLV precomputation script.
GLV impl for both batch affine and projective mul
Batch bucket addition tree
Batched MSM (1.4-1.7x bump)
Batch subgroup verification based on addition to random buckets
2D test matrix feature-gating with axes (curve, test type)
Refactor tests to use common template
Code timing instrumentation with fine grained control of functions and callers

Combined speedup (with w-NAF) for

Batched mul (with GLV): 2.3-2.7x.
Projective mul (with GLV): 2.1x.
Without GLV: 1.65-1.8x and 1.4x resp.

Speedup for subgroup verification

Over old impl 25-80x.
Over new batch mul: 10-30x.

TODO:

Extend ordinary w-NAF to non-GLV curves.

jon-chuang · 2020-09-11T04:56:23Z

TODO: add curve-specific parameters for batch ops:

prefetching number
batch size

kobigurk

Very cool we got to this stage. I know this is still WIP but I thought it's a good time to take a look.

General comments and questions:

Why was it important to add the prime_fields and extension_fields features?
Seems like the tests use extensions_fields in plural rather than singular.
GLV doesn't seem to be generic and so maybe we should assert on the parameters that actually work.
I'm a bit concerned about adding another timer mechanism. But if the benefits are large, I'm generally OK with it.

In general, this is a pretty big PR. In the future, if we want to make big changes, we should make a "tower of PRs", a series of PRs that build on top of each other. It makes it much easier to review.

kobigurk · 2020-09-13T20:53:50Z

algebra-core/build.rs

 const NUM_LIMBS: usize = 8;

 fn main() {
    println!("cargo:rerun-if-changed=build.rs");

    let is_nightly = version_meta().expect("nightly check failed").channel == Channel::Nightly;

-    let should_use_asm = cfg!(all(
+    let _should_use_asm = cfg!(all(


Is this still needed?

kobigurk · 2020-09-13T20:54:18Z

algebra-core/build.rs

        feature = "llvm_asm",
        target_feature = "bmi2",
        target_feature = "adx",
        target_arch = "x86_64"
    )) && is_nightly;
+
+    #[cfg(features = "llvm_asm")]
    if should_use_asm {


Does this compile if should_use_asm is undefined?

kobigurk · 2020-09-13T20:57:08Z

algebra-core/src/curves/glv.rs

+use crate::{biginteger::BigInteger, ModelParameters, PrimeField};
+use core::ops::Neg;
+
+/// TODO: deal with the case where b1 and b2 have the same sign


Do we worry we have a case like that?

Yes, because they can reverse sign. Empirically speaking. Due to the exceeding the modulus.

kobigurk · 2020-09-13T20:58:12Z

algebra-core/src/curves/glv.rs

+        // If we are doing a subgroup check, we should multiply by the original scalar
+        // since the GLV decomposition does not guarantee that we would not be
+        // adding and subtracting back to zero
+        if k == modulus {


Interesting.
We shouldn't do it now, but there are endomorphism-based methods for subgroup membership checking.

Yes, I am aware. It should also be possible to handle this case by using biginteger rather than field. I will look into whether it can be handled generically.

algebra-core/src/curves/models/bw6/mod.rs

algebra/src/cp6_782/fields/tests.rs

kobigurk · 2020-09-13T21:06:36Z

algebra/src/ed_on_mnt4_298/curves/tests.rs

-};
-use rand::Rng;
-
+#![allow(unused_imports)]


Why do we need to allow this?

kobigurk · 2020-09-13T21:07:45Z

algebra/src/tests/msm.rs

-    assert_eq!(naive.into_affine(), fast.into_affine());
-}
+#[allow(unused)]
+pub fn test_msm<G: AffineCurve>() {


Why is this unused now?

kobigurk · 2020-09-13T21:08:41Z

scripts/glv_lattice_basis/src/main.rs

+use std::ops::Neg;
+
+fn main() {
+    let _omega_g1 = BigInteger768([


Can you clarify where these numbers are from?

kobigurk · 2020-09-13T21:09:08Z

algebra/src/bw6_761/curves/g1.rs

+    type WideBigInt = BigInteger768;
+
+    /// phi((x, y)) = (\omega x, y)
+    /// \omega = 0x531dc16c6ecd27aa846c61024e4cca6c1f31e53bd9603c2d17be416c5e44


Can you clarify where these numbers are from?

jon-chuang · 2020-09-14T07:17:47Z

Very cool we got to this stage. I know this is still WIP but I thought it's a good time to take a look.

General comments and questions:

Why was it important to add the prime_fields and extension_fields features?

No particular reason. Sometimes the benchmarks can be messy. Nicer to look at pure Fp for instance to evaluate the asm impl.

Seems like the tests use extensions_fields in plural rather than singular.

Will change

GLV doesn't seem to be generic and so maybe we should assert on the parameters that actually work.

I prefer to rewrite as generic, as it comes hand in hand with checking the math. Target is done by wednesday.

I'm a bit concerned about adding another timer mechanism. But if the benefits are large, I'm generally OK with it.

It's something that is already integrated and works. We can migrate to another timer (tracing) in the future.

In general, this is a pretty big PR. In the future, if we want to make big changes, we should make a "tower of PRs", a series of PRs that build on top of each other. It makes it much easier to review.

This is true, but the problem is code drift. There was also a lot of refactorisation. A more pertinent solution may be to try to get merges more quickly.

Pratyush · 2020-09-15T04:26:48Z

To clean up the git history, and to make reviewing easier, I would recommend using something like git reset --soft XYZ to reset the git history to commit XYZ. This keeps the changes, but simply uncommits and unstages them. Then, you can add changes either by module, or interactively via git add -i. This way you can group logical changes together.

I took this approach with a recent PR (arkworks-rs#186)

jon-chuang · 2020-09-22T12:58:02Z

Workflow now:

Wait to merge updated master.
Then create branches no_glv, and just_tests, successively redacting glv and everything but tests.
Then quickly do final review and merge into celo-org.
Then, make 3 PRs to scipr-lab and recommend they be addressed one at a time...

jon-chuang · 2020-09-22T13:02:32Z

Also, need to change glv script to example first

jon-chuang · 2020-10-06T11:11:20Z

closed in favour of #16

jon-chuang added 30 commits August 1, 2020 00:46

First draft affine batch ops & wnaf

a64d7fb

changes to mutability and lifetimes

b7024dd

delete superfluous files

40ef5d7

crazy direction: Passing a FnMut to generate an iterator locally

0fa5eeb

unsuccessful further attempts

eebb12b

compile sucess using index approach

4d22acf

fixes for mutable borrows

bbbec75

Successfully passed scalar mul test

3a6e45c

benchmarks + prefetching

5c65917

stash

3bf2bc1

generic impl of batch arith for all affinecurves

4bb5ad5

batched affine formulas for TE - too expensive

67da071

improved TE affine

2e54f67

cleanup batch inversion

62df27d

fmt...

e6d28b6

fix minor error

74d9bb7

remove debugging scaffolding

908fb73

fmt...

c0a5a07

delete batch arith bench as not suitable for criterion or bench

5c89660

fix bench removal errors

6359f7c

fmt...

56b8181

added missing coeff_a

ec2decd

refactor BatchGroupArithmetic to be separate trait

bad37bd

Batch verification with radix sort

5b9cae9

Cache-locality & parallelisation

cbf8e49

Successfully impl batch verify

200f5fa

added tests and bench for batch_ver, parallel_random_gen, ^ thread util

ed7c4a7

fmt

0e612e4

enabled missing test

8819290

remove voracious_radix_sort

a8e9c18

jon-chuang added 6 commits September 10, 2020 04:16

Minor changes

1eacd89

refactor tests, generic MSM test

204ffa5

2D test matrix :)

9efaae4

batchaffine

bd82f31

tests

e5cb574

additive features

3ed5d9f

jon-chuang force-pushed the jonch/glv branch from 0df5203 to 3ed5d9f Compare September 11, 2020 00:02

big_n feature for test-benching

2fc20e4

jon-chuang added 2 commits September 11, 2020 15:11

prefetch unroll

f21f40a

minor adjustments

c605894

kobigurk suggested changes Sep 13, 2020

View reviewed changes

jon-chuang added 3 commits September 14, 2020 15:22

extension(s -> "")_fields

6a70b67

remove artifacts, fix asm

c83b29d

uncomment subgroup checks, glv param sources

3a8e853

jon-chuang closed this Sep 15, 2020

jon-chuang reopened this Sep 15, 2020

jon-chuang added 2 commits September 22, 2020 20:09

Clean up GLV murkiness and add comments

d8c5d08

Set defaults for glv_window_size

a5f4521

refactor glv to use examples

6b65eda

jon-chuang closed this Oct 6, 2020

kobigurk deleted the jonch/glv branch October 12, 2020 17:30

ghost mentioned this pull request Mar 19, 2021

Fast subgroup checks anoma/ferveo#14

Closed

imeckler mentioned this pull request Jan 13, 2022

Feature request: fast pippenger MSM algorithm as in barretenberg arkworks-rs/algebra#380

Open

Brechtpd mentioned this pull request May 7, 2022

MSM optimization privacy-scaling-explorations/halo2#40

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Alg Opt with GLV #4

Alg Opt with GLV #4

jon-chuang commented Sep 3, 2020 •

edited

Loading

jon-chuang commented Sep 11, 2020

kobigurk left a comment

kobigurk Sep 13, 2020

kobigurk Sep 13, 2020

kobigurk Sep 13, 2020

jon-chuang Sep 14, 2020

kobigurk Sep 13, 2020

jon-chuang Sep 14, 2020

kobigurk Sep 13, 2020

kobigurk Sep 13, 2020

kobigurk Sep 13, 2020

kobigurk Sep 13, 2020

jon-chuang commented Sep 14, 2020 •

edited

Loading

Pratyush commented Sep 15, 2020 •

edited

Loading

jon-chuang commented Sep 22, 2020

jon-chuang commented Sep 22, 2020

jon-chuang commented Oct 6, 2020

Alg Opt with GLV #4

Alg Opt with GLV #4

Conversation

jon-chuang commented Sep 3, 2020 • edited Loading

jon-chuang commented Sep 11, 2020

kobigurk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jon-chuang commented Sep 14, 2020 • edited Loading

Pratyush commented Sep 15, 2020 • edited Loading

jon-chuang commented Sep 22, 2020

jon-chuang commented Sep 22, 2020

jon-chuang commented Oct 6, 2020

jon-chuang commented Sep 3, 2020 •

edited

Loading

jon-chuang commented Sep 14, 2020 •

edited

Loading

Pratyush commented Sep 15, 2020 •

edited

Loading