-
Notifications
You must be signed in to change notification settings - Fork 30
chore(proto): Update to rand 0.10 #511
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
Changes from all commits
15f38df
6cf2f1a
33a185f
e8338fa
c8e6e7b
c8d765a
dcbcfdc
6cc2001
4746c17
2b322b9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,7 @@ | ||
| use std::hash::Hasher; | ||
|
|
||
| use rand::{Rng, RngCore}; | ||
| use rand::Rng; | ||
| use rand::RngExt; | ||
|
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. What's with our formatter? It is on strike? (genuinely the dig is at our tooling, this is not something you should have to handle)
Member
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. Yeah - weirdly enough running |
||
|
|
||
| use crate::Duration; | ||
| use crate::MAX_CID_SIZE; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,7 +2,7 @@ use std::any::Any; | |
| use std::fmt::Debug; | ||
| use std::sync::Arc; | ||
|
|
||
| use rand::{Rng, SeedableRng}; | ||
| use rand::RngExt; | ||
|
|
||
| use crate::congestion::ControllerMetrics; | ||
| use crate::congestion::bbr::bw_estimation::BandwidthEstimation; | ||
|
|
@@ -56,7 +56,6 @@ pub struct Bbr { | |
| bw_at_last_round: u64, | ||
| round_wo_bw_gain: u64, | ||
| ack_aggregation: AckAggregationState, | ||
| random_number_generator: rand::rngs::StdRng, | ||
| } | ||
|
|
||
| impl Bbr { | ||
|
|
@@ -97,7 +96,6 @@ impl Bbr { | |
| bw_at_last_round: 0, | ||
| round_wo_bw_gain: 0, | ||
| ack_aggregation: AckAggregationState::default(), | ||
| random_number_generator: rand::rngs::StdRng::from_os_rng(), | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -114,9 +112,7 @@ impl Bbr { | |
| // Pick a random offset for the gain cycle out of {0, 2..7} range. 1 is | ||
| // excluded because in that case increased gain and decreased gain would not | ||
| // follow each other. | ||
| let mut rand_index = self | ||
| .random_number_generator | ||
| .random_range(0..K_PACING_GAIN.len() as u8 - 1); | ||
| let mut rand_index = rand::rng().random_range(0..K_PACING_GAIN.len() as u8 - 1); | ||
|
Contributor
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. this is very unfortunate, I know we are planning on removing it, but we would need this back later, for proper seeded testing :/
Member
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 know - but the previous version of this code didn't allow passing in the randomness or a seed either, so this is no regression in that sense. I'm aware of this issue in general, but fixing this would be annoying and blow up the diff in this PR.
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. Are you not losing the option to be deterministic by doing this?
Member
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. You never had the option to configure this congestion controller to be deterministic in the first place, so no. |
||
| if rand_index >= 1 { | ||
| rand_index += 1; | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -8,7 +8,7 @@ use std::{ | |
| }; | ||
|
|
||
| use bytes::{Buf, BufMut, Bytes, BytesMut}; | ||
| use rand::{Rng, RngCore, SeedableRng, rngs::StdRng}; | ||
| use rand::{Rng, RngExt, SeedableRng, rngs::StdRng}; | ||
| use rustc_hash::FxHashMap; | ||
| use slab::Slab; | ||
| use thiserror::Error; | ||
|
|
@@ -69,7 +69,7 @@ impl Endpoint { | |
| Self { | ||
| rng: config | ||
| .rng_seed | ||
| .map_or_else(StdRng::from_os_rng, StdRng::from_seed), | ||
| .map_or_else(|| StdRng::from_rng(&mut rand::rng()), StdRng::from_seed), | ||
|
Contributor
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. what's the benefit of StdRng over rand::rng?
Member
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.
|
||
| index: ConnectionIndex::default(), | ||
| connections: Slab::new(), | ||
| local_cid_generator: (config.connection_id_generator_factory.as_ref())(), | ||
|
|
||
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.
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.
This is only in dev-dependencies, and only because we still depend on proptest, and we run some tests in Wasm that depend on proptest (although we don't run proptests themselves). This is a trade-off we made a while ago to make cfg-ing proptest things everywhere not necessary.