chore(network): stress test utils #1995
Conversation
|
This change is |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1995 +/- ##
==========================================
- Coverage 40.10% 34.65% -5.45%
==========================================
Files 26 56 +30
Lines 1895 4931 +3036
Branches 1895 4931 +3036
==========================================
+ Hits 760 1709 +949
- Misses 1100 3165 +2065
- Partials 35 57 +22 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @eitanm-starkware)
crates/papyrus_network/src/bin/network_stress_test/converters.rs line 8 at r1 (raw file):
pub payload: Vec<u8>, pub time: SystemTime, }
newline
crates/papyrus_network/src/bin/network_stress_test/converters.rs line 33 at r1 (raw file):
fn from(mut value: Vec<u8>) -> Self { let vec_size = value.len(); let payload_size = vec_size - 16;
Extract to a constant, and calculate it by using https://doc.rust-lang.org/std/mem/fn.size_of.html and addition
crates/papyrus_network/src/bin/network_stress_test/utils.rs line 18 at r1 (raw file):
pub network_config: NetworkConfig, pub buffer_size: usize, pub payload_size: usize,
Rename to message_size, and reduce the metadata size from it before using it to define the payload (use the constant from previous comment)
crates/papyrus_network/src/bin/network_stress_test/utils.rs line 19 at r1 (raw file):
pub buffer_size: usize, pub payload_size: usize, pub message_amount: u32,
rename to num_messages
crates/papyrus_network/src/bin/network_stress_test/utils.rs line 86 at r1 (raw file):
} pub fn system_time_to_millis<S>(time: &SystemTime, serializer: S) -> Result<S::Ok, S::Error>
This function both converts to u64 and serializes. IMO it shouldn't do both. If you disagree at least rename it to reflect that it does both
crates/papyrus_network/src/bin/network_stress_test/utils.rs line 93 at r1 (raw file):
time.duration_since(UNIX_EPOCH).map_err(serde::ser::Error::custom)?; let millis = duration_since_epoch.as_secs() * 1000 + u64::from(duration_since_epoch.subsec_millis());
Why not `duration_since_epoch.as_millis()
eitanm-starkware
force-pushed
the
eitan/nw_stress_test
branch
from
ca21836
to
9ab8bfb
Compare
There was a problem hiding this comment.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @ShahakShama)
crates/papyrus_network/src/bin/network_stress_test/converters.rs line 33 at r1 (raw file):
Previously, ShahakShama wrote…
Extract to a constant, and calculate it by using https://doc.rust-lang.org/std/mem/fn.size_of.html and addition
Done.
crates/papyrus_network/src/bin/network_stress_test/utils.rs line 19 at r1 (raw file):
Previously, ShahakShama wrote…
rename to num_messages
Done.
crates/papyrus_network/src/bin/network_stress_test/utils.rs line 86 at r1 (raw file):
Previously, ShahakShama wrote…
This function both converts to u64 and serializes. IMO it shouldn't do both. If you disagree at least rename it to reflect that it does both
Done.
crates/papyrus_network/src/bin/network_stress_test/utils.rs line 93 at r1 (raw file):
Previously, ShahakShama wrote…
Why not `duration_since_epoch.as_millis()
Done.
eitanm-starkware
force-pushed
the
eitan/nw_stress_test
branch
from
9ab8bfb
to
6f54838
Compare
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
| use std::time::{Duration, SystemTime}; | ||
|
|
||
| struct StressTestMessage { | ||
| id: u32, | ||
| payload: Vec<u8>, | ||
| time: SystemTime, | ||
| pub const METADATA_SIZE: usize = size_of::<SystemTime>() + size_of::<u32>(); |
There was a problem hiding this comment.
The size_of function is used but not imported. Please add use std::mem::size_of; to the imports at the top of the file.
Spotted by Graphite Reviewer
Is this helpful? React 👍 or 👎 to let us know.
There was a problem hiding this comment.
Reviewed 2 of 2 files at r2, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @eitanm-starkware and @graphite-app[bot])
crates/papyrus_network/src/bin/network_stress_test/utils.rs line 86 at r1 (raw file):
Previously, eitanm-starkware wrote…
Done.
This is not done
crates/papyrus_network/src/bin/network_stress_test/utils.rs line 93 at r1 (raw file):
Previously, eitanm-starkware wrote…
Done.
This is not done
| use std::time::{Duration, SystemTime}; | ||
|
|
||
| struct StressTestMessage { | ||
| id: u32, | ||
| payload: Vec<u8>, | ||
| time: SystemTime, | ||
| pub const METADATA_SIZE: usize = size_of::<SystemTime>() + size_of::<u32>(); |
eitanm-starkware
force-pushed
the
eitan/nw_stress_test
branch
from
6f54838
to
fb93f2a
Compare
| let millis = | ||
| duration_since_epoch.as_millis() + u128::from(duration_since_epoch.subsec_millis()); |
There was a problem hiding this comment.
The as_millis() method already includes the subsec_millis portion of the duration. Adding subsec_millis() again results in double-counting the milliseconds component. The line should be simplified to just duration_since_epoch.as_millis().
Spotted by Graphite Reviewer
Is this helpful? React 👍 or 👎 to let us know.
eitanm-starkware
force-pushed
the
eitan/nw_stress_test
branch
from
fb93f2a
to
be8e903
Compare
There was a problem hiding this comment.
Reviewed 2 of 2 files at r3, 3 of 3 files at r4, 2 of 2 files at r5, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @graphite-app[bot])
| let millis = | ||
| duration_since_epoch.as_millis() + u128::from(duration_since_epoch.subsec_millis()); |
eitanm-starkware
force-pushed
the
eitan/nw_stress_test
branch
from
be8e903
to
79248c0
Compare
There was a problem hiding this comment.
Reviewed 1 of 1 files at r6, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @graphite-app[bot])
There was a problem hiding this comment.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @graphite-app[bot])
No description provided.