Improve performance of recursive

[chiro-hiro]

Im try to minimize the cost to .copy(), .clone() multiple structures around. You might expect ~10% in total performance and ~5% in memory saving.

2 tests are broken, so I'm try to fix it. If this worth, please let me know.

[chiro-hiro]

@microsoft-github-policy-service agree company="OROCHI NETWORK PTE. LTD."

[srinathsetty]

@chiro-hiro Is this still a draft? Could you please rebase with the main branch and let me know when this is ready?

[chiro-hiro]

@chiro-hiro Is this still a draft? Could you please rebase with the main branch and let me know when this is ready?

Thank you, I will rebase my code and try to complete this in the weekend.

[chiro-hiro]

Here is the original code, there are multiple instance of RecursiveSNARK and the paramerters was clone each time we invoke .prove_step().

    // produce a recursive SNARK
    let mut recursive_snark: Option<
      RecursiveSNARK<
        G1,
        G2,
        TrivialTestCircuit<<G1 as Group>::Scalar>,
        CubicCircuit<<G2 as Group>::Scalar>,
      >,
    > = None;

    for _i in 0..num_steps {
      let res = RecursiveSNARK::prove_step(
        &pp,
        recursive_snark,
        circuit_primary.clone(),
        circuit_secondary.clone(),
        vec![<G1 as Group>::Scalar::ONE],
        vec![<G2 as Group>::Scalar::ZERO],
      );
      assert!(res.is_ok());
      recursive_snark = Some(res.unwrap());
    }

    assert!(recursive_snark.is_some());
    let recursive_snark = recursive_snark.unwrap();

    // verify the recursive SNARK
    let res = recursive_snark.verify(
      &pp,
      num_steps,
      vec![<G1 as Group>::Scalar::ONE],
      vec![<G2 as Group>::Scalar::ZERO],
    );
    assert!(res.is_ok());

Instead of create new instance of RecursiveSNARK every time, I changed the code to manage a single mutable instance of RecursiveSNARK.

    // produce a recursive SNARK
    let mut recursive_snark = RecursiveSNARK::<
      G1,
      G2,
      TrivialTestCircuit<<G1 as Group>::Scalar>,
      CubicCircuit<<G2 as Group>::Scalar>,
    >::new(
      &pp,
      &circuit_primary,
      &circuit_secondary,
      vec![<G1 as Group>::Scalar::one()],
      vec![<G2 as Group>::Scalar::zero()],
    );

    for _i in 0..num_steps {
      let res = recursive_snark.prove_step(
        &pp,
        &circuit_primary,
        &circuit_secondary,
        vec![<G1 as Group>::Scalar::one()],
        vec![<G2 as Group>::Scalar::zero()],
      );
      assert!(res.is_ok());
    }

    // verify the recursive SNARK
    let res = recursive_snark.verify(
      &pp,
      num_steps,
      vec![<G1 as Group>::Scalar::ONE],
      vec![<G2 as Group>::Scalar::ZERO],
    );
    assert!(res.is_ok());

In my observations, the circuit and z0 were passed as immutable parameters. I'm also change the code to borrow these values instead of .clone() or copy.

[srinathsetty]

Fantastic! Thanks @chiro-hiro! I had a quick comment about avoiding a small piece of code in prove_step. Please check.

[chiro-hiro]

My pleasure,

Indeed, that part of code looks ugly but I can't find the better way to keep the backwards compatibility.

[chiro-hiro]

Hi @srinathsetty, there are pretty much changes were made in main branch. It's a bit overwhelming to keep my branch compatible. Could we speed up the process for this PR?.

[srinathsetty]

Hi @srinathsetty, there are pretty much changes were made in main branch. It's a bit overwhelming to keep my branch compatible. Could we speed up the process for this PR?.

Hi @chiro-hiro! I'll see if I can find a fix for the issue noted above. We'll prioritize this PR.

[chiro-hiro]

Thank you, I will focus on fixing the conflict with the main branch.

[huitseeker]

@chiro-hiro this is neat! I've opened orochi-network#1 to refine it further - you might find this or a similar way of getting the same results interesting.

[chiro-hiro]

@huitseeker Thank you, everything looks great. I do check and rerun the test/benchmark as well. All set, just wait for @srinathsetty to review this PR.

[huitseeker]

@chiro-hiro clippy warnings should disappear as soon as you rebase on the latest master.

[srinathsetty]

Hi, @chiro-hiro Could we please update this branch soon (with a rebase + a change to address the discussion thread above) so we can merge this ahead of other pending PRs? (cc: @huitseeker)

[chiro-hiro]

Hi @srinathsetty, sorry for the delay. I have rebasing my fork with the upstream's main branch.