Skip to content
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

Clarify Protogalaxy state #881

Open
codygunton opened this issue Feb 29, 2024 · 0 comments
Open

Clarify Protogalaxy state #881

codygunton opened this issue Feb 29, 2024 · 0 comments

Comments

@codygunton
Copy link
Collaborator

codygunton commented Feb 29, 2024

We should clarify and simplify the handling of state in Protogalaxy. Examples:

  • auto is overused and makes it hard to navigate.
  • the name "accumulator" is used for objects that are not being treated as an accumulator (the idea is that these objects have the same type as an accumulator, but IMO the naming here is confusing and should be improved)
  • the is_accumulator flag in the ProverInstance is a bit of a misnomer--I believe this sometimes is more properly an "is_first_round".
  • There is a bad pattern in that part of the code where objects are default constructed when probably they should be copy constructed.

Example of the final point: I tried to rewrite

auto next_accumulator = std::make_shared<Instance>();
next_accumulator->is_accumulator = true;
next_accumulator->instance_size = instances[0]->instance_size;
next_accumulator->log_instance_size = instances[0]->log_instance_size;

to

auto next_accumulator = instances[0];
next_accumulator->is_accumulator = true;
next_accumulator->instance_size = instances[0]->instance_size;
next_accumulator->log_instance_size = instances[0]->log_instance_size;

in order to give next_accumulator a copy of the commitment key. This ran into trouble (Decider tests failed) but

auto next_accumulator = std::make_shared<Instance>();
next_accumulator->is_accumulator = true;
next_accumulator->instance_size = instances[0]->instance_size;
next_accumulator->log_instance_size = instances[0]->log_instance_size;
next_accumulator->commitment_key = instances[0]->commitment_key;

works. IMO this is clear example of brittleness in our handling of state.

codygunton pushed a commit to AztecProtocol/aztec-packages that referenced this issue Apr 1, 2024
Partially addresses
AztecProtocol/barretenberg#881.

Cuts down on accumulator_update_round time by 300ms!

```
--------------------------------------------------------------------------------
Benchmark                      Time             CPU   Iterations UserCounters...
--------------------------------------------------------------------------------
ClientIVCBench/Full/6      22272 ms        17339 ms            1 Decider::construct_proof=1 Decider::construct_proof(t)=752.81M ECCVMComposer::compute_commitment_key=1 ECCVMComposer::compute_commitment_key(t)=3.74397M ECCVMComposer::compute_witness=1 ECCVMComposer::compute_witness(t)=131.409M ECCVMComposer::create_prover=1 ECCVMComposer::create_prover(t)=151.452M ECCVMComposer::create_proving_key=1 ECCVMComposer::create_proving_key(t)=16.0709M ECCVMProver::construct_proof=1 ECCVMProver::construct_proof(t)=1.76787G Goblin::merge=11 Goblin::merge(t)=144.332M GoblinTranslatorCircuitBuilder::constructor=1 GoblinTranslatorCircuitBuilder::constructor(t)=58.9093M GoblinTranslatorProver=1 GoblinTranslatorProver(t)=164.575M GoblinTranslatorProver::construct_proof=1 GoblinTranslatorProver::construct_proof(t)=958.34M ProtoGalaxyProver_::accumulator_update_round=10 ProtoGalaxyProver_::accumulator_update_round(t)=292.705M ProtoGalaxyProver_::combiner_quotient_round=10 ProtoGalaxyProver_::combiner_quotient_round(t)=5.96917G ProtoGalaxyProver_::perturbator_round=10 ProtoGalaxyProver_::perturbator_round(t)=1.30937G ProtoGalaxyProver_::preparation_round=10 ProtoGalaxyProver_::preparation_round(t)=4.18574G ProtogalaxyProver::fold_instances=10 ProtogalaxyProver::fold_instances(t)=11.757G ProverInstance(Circuit&)=11 ProverInstance(Circuit&)(t)=1.99482G batch_mul_with_endomorphism=30 batch_mul_with_endomorphism(t)=566.111M commit=426 commit(t)=4.0365G compute_combiner=10 compute_combiner(t)=5.96705G compute_perturbator=9 compute_perturbator(t)=1.30905G compute_univariate=48 compute_univariate(t)=1.42119G construct_circuits=6 construct_circuits(t)=4.49152G
Benchmarking lock deleted.
client_ivc_bench.json                                                            100% 3992   123.3KB/s   00:00    
function                                        ms     % sum
construct_circuits(t)                         4492    20.40%
ProverInstance(Circuit&)(t)                   1995     9.06%
ProtogalaxyProver::fold_instances(t)         11757    53.40%
Decider::construct_proof(t)                    753     3.42%
ECCVMComposer::create_prover(t)                151     0.69%
ECCVMProver::construct_proof(t)               1768     8.03%
GoblinTranslatorProver::construct_proof(t)     958     4.35%
Goblin::merge(t)                               144     0.66%

Total time accounted for: 22018ms/22272ms = 98.86%

Major contributors:
function                                        ms    % sum
commit(t)                                     4037   18.33%
compute_combiner(t)                           5967   27.10%
compute_perturbator(t)                        1309    5.95%
compute_univariate(t)                         1421    6.45%

Breakdown of ProtogalaxyProver::fold_instances:
ProtoGalaxyProver_::preparation_round(t)           4186    35.60%
ProtoGalaxyProver_::perturbator_round(t)           1309    11.14%
ProtoGalaxyProver_::combiner_quotient_round(t)     5969    50.77%
ProtoGalaxyProver_::accumulator_update_round(t)     293     2.49%
```

Please read [contributing guidelines](CONTRIBUTING.md) and remove this
line.
AztecBot pushed a commit that referenced this issue Apr 2, 2024
Partially addresses
#881.

Cuts down on accumulator_update_round time by 300ms!

```
--------------------------------------------------------------------------------
Benchmark                      Time             CPU   Iterations UserCounters...
--------------------------------------------------------------------------------
ClientIVCBench/Full/6      22272 ms        17339 ms            1 Decider::construct_proof=1 Decider::construct_proof(t)=752.81M ECCVMComposer::compute_commitment_key=1 ECCVMComposer::compute_commitment_key(t)=3.74397M ECCVMComposer::compute_witness=1 ECCVMComposer::compute_witness(t)=131.409M ECCVMComposer::create_prover=1 ECCVMComposer::create_prover(t)=151.452M ECCVMComposer::create_proving_key=1 ECCVMComposer::create_proving_key(t)=16.0709M ECCVMProver::construct_proof=1 ECCVMProver::construct_proof(t)=1.76787G Goblin::merge=11 Goblin::merge(t)=144.332M GoblinTranslatorCircuitBuilder::constructor=1 GoblinTranslatorCircuitBuilder::constructor(t)=58.9093M GoblinTranslatorProver=1 GoblinTranslatorProver(t)=164.575M GoblinTranslatorProver::construct_proof=1 GoblinTranslatorProver::construct_proof(t)=958.34M ProtoGalaxyProver_::accumulator_update_round=10 ProtoGalaxyProver_::accumulator_update_round(t)=292.705M ProtoGalaxyProver_::combiner_quotient_round=10 ProtoGalaxyProver_::combiner_quotient_round(t)=5.96917G ProtoGalaxyProver_::perturbator_round=10 ProtoGalaxyProver_::perturbator_round(t)=1.30937G ProtoGalaxyProver_::preparation_round=10 ProtoGalaxyProver_::preparation_round(t)=4.18574G ProtogalaxyProver::fold_instances=10 ProtogalaxyProver::fold_instances(t)=11.757G ProverInstance(Circuit&)=11 ProverInstance(Circuit&)(t)=1.99482G batch_mul_with_endomorphism=30 batch_mul_with_endomorphism(t)=566.111M commit=426 commit(t)=4.0365G compute_combiner=10 compute_combiner(t)=5.96705G compute_perturbator=9 compute_perturbator(t)=1.30905G compute_univariate=48 compute_univariate(t)=1.42119G construct_circuits=6 construct_circuits(t)=4.49152G
Benchmarking lock deleted.
client_ivc_bench.json                                                            100% 3992   123.3KB/s   00:00    
function                                        ms     % sum
construct_circuits(t)                         4492    20.40%
ProverInstance(Circuit&)(t)                   1995     9.06%
ProtogalaxyProver::fold_instances(t)         11757    53.40%
Decider::construct_proof(t)                    753     3.42%
ECCVMComposer::create_prover(t)                151     0.69%
ECCVMProver::construct_proof(t)               1768     8.03%
GoblinTranslatorProver::construct_proof(t)     958     4.35%
Goblin::merge(t)                               144     0.66%

Total time accounted for: 22018ms/22272ms = 98.86%

Major contributors:
function                                        ms    % sum
commit(t)                                     4037   18.33%
compute_combiner(t)                           5967   27.10%
compute_perturbator(t)                        1309    5.95%
compute_univariate(t)                         1421    6.45%

Breakdown of ProtogalaxyProver::fold_instances:
ProtoGalaxyProver_::preparation_round(t)           4186    35.60%
ProtoGalaxyProver_::perturbator_round(t)           1309    11.14%
ProtoGalaxyProver_::combiner_quotient_round(t)     5969    50.77%
ProtoGalaxyProver_::accumulator_update_round(t)     293     2.49%
```

Please read [contributing guidelines](CONTRIBUTING.md) and remove this
line.
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 a pull request may close this issue.

3 participants