-
Notifications
You must be signed in to change notification settings - Fork 2
Benchmarks with latency #240
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
base: main
Are you sure you want to change the base?
Changes from all commits
edf21ec
ac58781
f8966a2
d35d317
11db1da
5bcd605
47f4fed
fab6283
a685499
63c02be
4bee396
f7a34c0
3720e8f
358354d
1faa085
4579ade
4d6c869
d14256d
cd35a52
8b73048
2c906ef
2795722
2e6172d
fedaae3
5ba73f8
e6942f8
3022772
73144e7
d90429c
70bb1f9
a745950
224cb01
1d91be3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -169,11 +169,16 @@ pub fn run_two_party_protocol<T0: std::fmt::Debug, T1: std::fmt::Debug>( | |
|
|
||
| /// Runs one real participant and one simulation representing the rest of participants | ||
| /// The simulation has an internal storage of what to send to the real participant | ||
| /// | ||
| /// Accepts a network latency in milliseconds say 100ms | ||
| pub fn run_simulated_protocol<T>( | ||
| real_participant: Participant, | ||
| mut real_prot: Box<dyn Protocol<Output = T>>, | ||
| simulator: Simulator, | ||
| network_latency: u64, | ||
| round_number: u64, | ||
| ) -> Result<T, ProtocolError> { | ||
| let duration = std::time::Duration::from_millis(network_latency * round_number); | ||
| if simulator.real_participant() != real_participant { | ||
| return Err(ProtocolError::AssertionFailed( | ||
| "The given real participant does not match the simulator's internal real participant" | ||
|
|
@@ -193,5 +198,6 @@ pub fn run_simulated_protocol<T>( | |
| out = Some(output); | ||
| } | ||
| } | ||
| std::thread::sleep(duration); | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why are we sleeping after running the protocol? I was under the impression that we wanted to inject latency in the networking layer, i.e. add a delay between sending and receiving messages. In what sense does this help us simulate network latency?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In a prior implementation, I tried injecting latency in the networking layer, but this was implying very weird numbers in the triple generation.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I suggest we merge this version of latency implementation as a simulation. Once I have a clear idea and I hope a better solution than what has been implemented earlier, then I can have a new PR. Note that the previous implementation was giving good numbers for presign and sign but not for triple gen. So I still prefer to have something right but simulated instead of something not fully right but "real" |
||
| out.ok_or_else(|| ProtocolError::Other("out is None".to_string())) | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Before accepting or refusing this change I need to know what was the idea to include it. Before we had a constant time duration for the benchmark, and now we are letting the users set the sample size. As the parameter input method we use are not great (we are forced to use env variables by criterion), then I would prefer to remain as before (const total time) or move to a constant number of samples, or use criterion's default, instead of making the implementation more verbose.
If we have a good reason to add sample size let me know.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Previously, the benchmark was not constant time. The time we fixed is an estimation. The code would automatically readjust the time it needs if the numbers we gave were smaller than what it actually needs.
Now why changing this? The benchmarkings were taking 40 or 45 min to run 100 iterations each. This was too wasteful for time and resources. Now the user can fix the number of iterations. For large protocols (like the one we have), it is absolutely acceptable to have 15 iterations as the bench numbers are quite exact (we talk of hundreds of milliseconds).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, I thought the previous
group.measurement_time(std::time::Duration::from_secs(300));referred to maximum allowed total time. From criterion docs:so why instead we put a constant, say 15 as you mention, for the benchmarks that we know otherwise would take too long with the default value, and leave the rest as is? I cannot think of a user that would want a different amount of samples, and worst case they could simply fork, modify and run.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you mean by leave the rest as is? If you're talking about simpler benches like scalar inversion or scalar multiplication, 15 iterations may lead to large variance.
For the rest, it is still giving the choice for the user to decide. A dummy example is me using it with lower number of sampling when I want to debug the bench XD
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I guess this is really a shortcoming of criterion, not allowing to set the sample size in the command line, which for debugging purposes would make total sense.
EDIT: found a criterion option exactly for this:
see the docs https://criterion-rs.github.io/book/user_guide/profiling.html?highlight=profile-time#--profile-time, it does exactly what you need if you want to run the benchmark for a short period of time during debug.
I think for general users that will not modify the code, we should just assign proper number of samples/max time per benchmark, as it will certainly differ depending on it