Adding telescope benchmarks #55

rrtoledo · 2024-11-06T13:13:16Z

Content

Relates to #54

Depends on #105

rrtoledo · 2024-11-11T13:26:39Z

Should we remove the benchmarks checking the variance of the number of repetitions (the dfs_bound is so high that we never repeat, which makes criterion fails when generating the plot), or should we lower dfs_bound to observe some variances in retry_counter?
-> Do we want the retry_counter to increase?

Some local tests showed me that we would need to reduce dfs_bound by a factor 100. Choosing this number would feel a bit arbitrary (Is this the right number for all lambdas...?) so I am not sure it is the best option.

tolikzinovyev · 2024-11-11T15:37:43Z

@tolikzinovyev @curiecrypt @djetchev

Should we remove the benchmarks checking the variance of the number of repetitions (the dfs_bound is so high that we never repeat, which makes criterion fails when generating the plot), or should we lower dfs_bound to observe some variances in retry_counter? -> Do we want the retry_counter to increase?

Some local tests showed me that we would need to reduce dfs_bound by a factor 100. Choosing this number would feel a bit arbitrary (Is this the right number for all lambdas...?) so I am not sure it is the best option.

Removing the benchmark sounds reasonable!

rrtoledo · 2024-11-13T11:17:43Z

I find @tolikzinovyev idea the best. What do you think @curiecrypt @djetchev ?

tolikzinovyev · 2024-11-14T05:22:28Z

I will not have time to review this PR before Monday, unfortunately, since it is a pretty large.

tolikzinovyev

Nice usage of advanced criterion features. Left high-level comments.

benches/centralized_telescope.rs

benches/criterion_helpers.rs

src/centralized_telescope/algorithm.rs

benches/centralized_telescope.rs

curiecrypt

I think it seems pretty good and mature. I left a few comments and could go for another round after the conversations are resolved.

Cargo.toml

benches/centralized_telescope.rs

benches/criterion_helpers.rs

src/centralized_telescope/algorithm.rs

benches/common/test_vectors.rs

curiecrypt

LGTM 👍

tolikzinovyev

The code structure, benchmarks duration and variable names have definitely improved, thank you!

src/centralized_telescope/algorithm.rs

tolikzinovyev · 2024-12-27T16:18:38Z

src/centralized_telescope/wrapper.rs

@@ -8,7 +8,8 @@ use crate::utils::types::Element;
 /// The main centralized Telescope struct with prove and verify functions.
 #[derive(Debug, Clone, Copy)]
 pub struct Wrapper {
-    setup: Setup,
+    /// Centralized telescope internal parameters
+    pub setup: Setup,


I think this shouldn't be public. Instead of calling CentralizedTelescope::create(), we can run parameter derivation and use CentralizedTelescope::create_unsafe().

The problem is accessing setup from the wrapper and not creating an unsafe wrapper from some user parameters.

I removed the pub access and added a getter function in the wrapper

I don't understand. Why not use CentralizedTelescope::create_unsafe() instead of modifying Wrapper? I would prefer to leave the public interface unchanged.

Just to be clear, I changed back the setup to private which now is unchanged.

The benches/centralized_telescope/utils.rs file comprises the setup function that calls the setup and returns both the dataset to bench on and a wrapper instance.
This function is used directly, with no changes in benches\...\proving_time.rs, and we call prove on the wrapper.
In the step_bench however, we need to call the bench function. As the function bench is defined in algorithm (outside the wrapper), it takes a setup as input. The setup was already done beforehand, and it seems more natural to extract the setup from the wrapper than compute it another time.

You could return Setup in benches/centralized_telescope/utils.rs setup() instead of CentralizedTelescope?

The getters have already been merged in another PR as @curiecrypt needed them for the examples.
As such, the two ways are equivalent.

benches/centralized_telescope/verifying_time.rs

benches/centralized_telescope/proof_size.rs

benches/common/criterion_helpers.rs

src/centralized_telescope/algorithm.rs

benches/centralized_telescope/number_steps.rs

benches/common/test_vectors.rs

curiecrypt

It looks quite mature. I just added a few minor comments.
I suggest merging it asap and dealing with the minor issues later.

benches/common/criterion_helpers.rs

src/centralized_telescope/algorithm.rs

benches/common/criterion_helpers.rs

benches/centralized_telescope/utils.rs

benches/centralized_telescope/number_steps.rs

src/centralized_telescope/algorithm.rs

benches/centralized_telescope/number_steps.rs

benches/centralized_telescope/proof_size.rs

benches/centralized_telescope/number_steps.rs

curiecrypt

LGTM 👍

tolikzinovyev · 2025-01-21T16:27:04Z

src/centralized_telescope/proof.rs

+    /// }
+    /// let (setps, proof_opt) = Proof::bench(set_size, &params, &prover_set);
+    /// ```
+    pub fn bench(set_size: u64, params: &Params, prover_set: &[Element]) -> (u64, Option<Proof>) {


Something happened to the internal module. I think the long documentation above is not necessary since this function is not supposed to be used.

We cannot have internal modules in a impl unfortunately. As the function needs to be accessed, it is public hence I added the doc for public functions.

I see. Then suggest replace the long function doc with "Alba's proving algorithm used for benchmarking, returning a proof if found as well as the number of steps done when generating it. Only for internal usage. Do not use." since it's not meant to be used publicly.

rrtoledo added the enhancement New feature or request label Nov 6, 2024

rrtoledo self-assigned this Nov 6, 2024

rrtoledo marked this pull request as draft November 6, 2024 13:13

rrtoledo linked an issue Nov 6, 2024 that may be closed by this pull request

Adding centralized benchmarks #54

Open

rrtoledo force-pushed the raph@bench-centralized branch 2 times, most recently from de0fb2a to 88d69ab Compare November 11, 2024 12:47

rrtoledo marked this pull request as ready for review November 11, 2024 13:22

rrtoledo requested review from tolikzinovyev, djetchev and curiecrypt November 13, 2024 11:17

tolikzinovyev reviewed Nov 19, 2024

View reviewed changes

curiecrypt reviewed Nov 20, 2024

View reviewed changes

rrtoledo requested review from curiecrypt and tolikzinovyev November 26, 2024 15:27

rrtoledo force-pushed the raph@bench-centralized branch from 14f4fb0 to 3fdfe36 Compare December 5, 2024 20:08

rrtoledo marked this pull request as draft December 5, 2024 20:08

rrtoledo force-pushed the raph@bench-centralized branch 2 times, most recently from 9f13518 to e27b0d4 Compare December 10, 2024 19:16

rrtoledo marked this pull request as ready for review December 12, 2024 12:01

rrtoledo force-pushed the raph@bench-centralized branch 4 times, most recently from b540e25 to a54996e Compare December 12, 2024 16:15

rrtoledo commented Dec 12, 2024

View reviewed changes

benches/common/test_vectors.rs Outdated Show resolved Hide resolved

rrtoledo changed the title ~~Adding benchmarks~~ Centralized Telescope - Adding benchmarks Dec 13, 2024

rrtoledo force-pushed the raph@bench-centralized branch from 63a4cef to 07fc567 Compare December 13, 2024 11:09

rrtoledo changed the title ~~Adding telescope & lottery benchmarks~~ Adding telescope benchmarks Dec 13, 2024

rrtoledo mentioned this pull request Dec 13, 2024

Bench lottery #107

Open

rrtoledo force-pushed the raph@bench-centralized branch from 350cc03 to f422cdb Compare December 16, 2024 11:06

curiecrypt approved these changes Dec 16, 2024

View reviewed changes

tolikzinovyev reviewed Dec 27, 2024

View reviewed changes

curiecrypt previously approved these changes Jan 9, 2025

View reviewed changes

rrtoledo dismissed curiecrypt’s stale review via 60d6ab9 January 14, 2025 20:36

djetchev reviewed Jan 17, 2025

View reviewed changes

benches/centralized_telescope/number_steps.rs Outdated Show resolved Hide resolved

rrtoledo force-pushed the raph@bench-centralized branch 2 times, most recently from 55ace81 to 13fcb6c Compare January 21, 2025 15:47

tolikzinovyev reviewed Jan 21, 2025

View reviewed changes

benches/centralized_telescope/proof_size.rs Outdated Show resolved Hide resolved

benches/centralized_telescope/number_steps.rs Outdated Show resolved Hide resolved

rrtoledo force-pushed the raph@bench-centralized branch from 13fcb6c to 2c0111d Compare January 21, 2025 15:48

curiecrypt previously approved these changes Jan 21, 2025

View reviewed changes

rrtoledo dismissed curiecrypt’s stale review via deb480b January 21, 2025 16:24

rrtoledo force-pushed the raph@bench-centralized branch from 2c0111d to deb480b Compare January 21, 2025 16:24

tolikzinovyev reviewed Jan 21, 2025

View reviewed changes

rrtoledo force-pushed the raph@bench-centralized branch from aa596ec to e4a3b75 Compare January 21, 2025 17:17

rrtoledo added 12 commits January 23, 2025 22:22

Tests/utils: renaming utils as common as menionned in rust book

010af2c

Benches/*: Adding mod and crierion_helpers

c61186e

Benches/CentralizedTelescope: Adding shared utils

a3a0318

Benches/Common: Adding test vectors

848547d

Benches/CentralizedTelescope: Adding proving time bench

b27d3b0

Benches/CentralizedTelescope: Adding verification time bench

5c30943

CentralizedTelescop/proof: Adding bench and prove_routine functions

5fcd392

Benches/CentralizedTelescope: Adding proving steps bench

e1defbc

Benches/CentralizedTelescope: Adding proving size bench

cc21230

Benches/Centralized/ProofSize: Updating prints

2fa4363

Benches/Centralized/NbSteps: Run bench once and multiply by nb_samples

bb1849b

Benches/TestVectors: Updating values to Peras only

f91b110

rrtoledo force-pushed the raph@bench-centralized branch from e4a3b75 to f91b110 Compare January 23, 2025 22:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding telescope benchmarks #55

Adding telescope benchmarks #55

rrtoledo commented Nov 6, 2024 •

edited

Loading

rrtoledo commented Nov 11, 2024

tolikzinovyev commented Nov 11, 2024

rrtoledo commented Nov 13, 2024

tolikzinovyev commented Nov 14, 2024

tolikzinovyev left a comment

curiecrypt left a comment

curiecrypt left a comment

tolikzinovyev left a comment

tolikzinovyev Dec 27, 2024

rrtoledo Jan 14, 2025

tolikzinovyev Jan 16, 2025

rrtoledo Jan 16, 2025

tolikzinovyev Jan 21, 2025

rrtoledo Jan 21, 2025

curiecrypt left a comment

curiecrypt left a comment

tolikzinovyev Jan 21, 2025

rrtoledo Jan 21, 2025

tolikzinovyev Jan 22, 2025

Adding telescope benchmarks #55

Are you sure you want to change the base?

Adding telescope benchmarks #55

Conversation

rrtoledo commented Nov 6, 2024 • edited Loading

Content

rrtoledo commented Nov 11, 2024

tolikzinovyev commented Nov 11, 2024

rrtoledo commented Nov 13, 2024

tolikzinovyev commented Nov 14, 2024

tolikzinovyev left a comment

Choose a reason for hiding this comment

curiecrypt left a comment

Choose a reason for hiding this comment

curiecrypt left a comment

Choose a reason for hiding this comment

tolikzinovyev 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

curiecrypt left a comment

Choose a reason for hiding this comment

curiecrypt 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

rrtoledo commented Nov 6, 2024 •

edited

Loading