From ccc8f6cc3debf7276cbdff78c7a906be6221ca35 Mon Sep 17 00:00:00 2001 From: Shawn Tabrizi Date: Tue, 11 Oct 2022 14:41:43 -0400 Subject: [PATCH] Avoid Unstable Sort (#12455) * dont use unstable sort * remove comment * add clippy rule --- .cargo/config.toml | 5 +++-- bin/node/bench/src/core.rs | 2 +- client/rpc-servers/src/lib.rs | 2 +- frame/benchmarking/src/analysis.rs | 4 ++-- frame/contracts/src/wasm/runtime.rs | 6 +----- frame/election-provider-multi-phase/src/mock.rs | 2 +- frame/examples/basic/src/benchmarking.rs | 2 +- primitives/npos-elections/fuzzer/src/common.rs | 6 +++--- utils/frame/benchmarking-cli/src/shared/stats.rs | 2 +- 9 files changed, 14 insertions(+), 17 deletions(-) diff --git a/.cargo/config.toml b/.cargo/config.toml index 5355758f7a4fa..66b28b3485d86 100644 --- a/.cargo/config.toml +++ b/.cargo/config.toml @@ -18,14 +18,15 @@ rustflags = [ "-Aclippy::borrowed-box", # Reasonable to fix this one "-Aclippy::too-many-arguments", # (Turning this on would lead to) "-Aclippy::unnecessary_cast", # Types may change - "-Aclippy::identity-op", # One case where we do 0 + + "-Aclippy::identity-op", # One case where we do 0 + "-Aclippy::useless_conversion", # Types may change "-Aclippy::unit_arg", # styalistic. "-Aclippy::option-map-unit-fn", # styalistic - "-Aclippy::bind_instead_of_map", # styalistic + "-Aclippy::bind_instead_of_map", # styalistic "-Aclippy::erasing_op", # E.g. 0 * DOLLARS "-Aclippy::eq_op", # In tests we test equality. "-Aclippy::while_immutable_condition", # false positives "-Aclippy::needless_option_as_deref", # false positives "-Aclippy::derivable_impls", # false positives + "-Aclippy::stable_sort_primitive", # prefer stable sort ] diff --git a/bin/node/bench/src/core.rs b/bin/node/bench/src/core.rs index 3b3060a888349..b6ad3ecd80068 100644 --- a/bin/node/bench/src/core.rs +++ b/bin/node/bench/src/core.rs @@ -132,7 +132,7 @@ pub fn run_benchmark(benchmark: Box, mode: Mode) -> Be durations.push(duration.as_nanos()); } - durations.sort_unstable(); + durations.sort(); let raw_average = (durations.iter().sum::() / (durations.len() as u128)) as u64; let average = (durations.iter().skip(10).take(30).sum::() / 30) as u64; diff --git a/client/rpc-servers/src/lib.rs b/client/rpc-servers/src/lib.rs index 0e2aca0dcc829..7eb825e169bfa 100644 --- a/client/rpc-servers/src/lib.rs +++ b/client/rpc-servers/src/lib.rs @@ -198,7 +198,7 @@ fn format_allowed_hosts(addrs: &[SocketAddr]) -> Vec { fn build_rpc_api(mut rpc_api: RpcModule) -> RpcModule { let mut available_methods = rpc_api.method_names().collect::>(); - available_methods.sort_unstable(); + available_methods.sort(); rpc_api .register_method("rpc_methods", move |_, _| { diff --git a/frame/benchmarking/src/analysis.rs b/frame/benchmarking/src/analysis.rs index 9ba2ea657bab0..a736cdc203182 100644 --- a/frame/benchmarking/src/analysis.rs +++ b/frame/benchmarking/src/analysis.rs @@ -181,7 +181,7 @@ impl Analysis { }) .collect(); - values.sort_unstable(); + values.sort(); let mid = values.len() / 2; Some(Self { @@ -311,7 +311,7 @@ impl Analysis { } for (_, rs) in results.iter_mut() { - rs.sort_unstable(); + rs.sort(); let ql = rs.len() / 4; *rs = rs[ql..rs.len() - ql].to_vec(); } diff --git a/frame/contracts/src/wasm/runtime.rs b/frame/contracts/src/wasm/runtime.rs index 3296492994071..6d7e6bcf69e5f 100644 --- a/frame/contracts/src/wasm/runtime.rs +++ b/frame/contracts/src/wasm/runtime.rs @@ -1976,11 +1976,7 @@ pub mod env { data_len: u32, ) -> Result<(), TrapReason> { fn has_duplicates(items: &mut Vec) -> bool { - // # Warning - // - // Unstable sorts are non-deterministic across architectures. The usage here is OK - // because we are rejecting duplicates which removes the non determinism. - items.sort_unstable(); + items.sort(); // Find any two consecutive equal elements. items.windows(2).any(|w| match &w { &[a, b] => a == b, diff --git a/frame/election-provider-multi-phase/src/mock.rs b/frame/election-provider-multi-phase/src/mock.rs index 6a638c8d2a2e1..e04e0bf474caf 100644 --- a/frame/election-provider-multi-phase/src/mock.rs +++ b/frame/election-provider-multi-phase/src/mock.rs @@ -155,7 +155,7 @@ pub fn trim_helpers() -> TrimHelpers { seq_phragmen(desired_targets as usize, targets.clone(), voters.clone(), None).unwrap(); // sort by decreasing order of stake - assignments.sort_unstable_by_key(|assignment| { + assignments.sort_by_key(|assignment| { std::cmp::Reverse(stakes.get(&assignment.who).cloned().unwrap_or_default()) }); diff --git a/frame/examples/basic/src/benchmarking.rs b/frame/examples/basic/src/benchmarking.rs index 4d1659af46460..87d65a0bfa5b6 100644 --- a/frame/examples/basic/src/benchmarking.rs +++ b/frame/examples/basic/src/benchmarking.rs @@ -63,7 +63,7 @@ benchmarks! { } }: { // The benchmark execution phase could also be a closure with custom code - m.sort_unstable(); + m.sort(); } // This line generates test cases for benchmarking, and could be run by: diff --git a/primitives/npos-elections/fuzzer/src/common.rs b/primitives/npos-elections/fuzzer/src/common.rs index e5853f28c4929..ad9bd43f9bce0 100644 --- a/primitives/npos-elections/fuzzer/src/common.rs +++ b/primitives/npos-elections/fuzzer/src/common.rs @@ -80,7 +80,7 @@ pub fn generate_random_npos_inputs( } candidates.push(id); } - candidates.sort_unstable(); + candidates.sort(); candidates.dedup(); assert_eq!(candidates.len(), candidate_count); @@ -99,11 +99,11 @@ pub fn generate_random_npos_inputs( let mut chosen_candidates = Vec::with_capacity(n_candidates_chosen); chosen_candidates.extend(candidates.choose_multiple(&mut rng, n_candidates_chosen)); - chosen_candidates.sort_unstable(); + chosen_candidates.sort(); voters.push((id, vote_weight, chosen_candidates)); } - voters.sort_unstable(); + voters.sort(); voters.dedup_by_key(|(id, _weight, _chosen_candidates)| *id); assert_eq!(voters.len(), voter_count); diff --git a/utils/frame/benchmarking-cli/src/shared/stats.rs b/utils/frame/benchmarking-cli/src/shared/stats.rs index 3234d5f2f94f7..ffae4a17724f8 100644 --- a/utils/frame/benchmarking-cli/src/shared/stats.rs +++ b/utils/frame/benchmarking-cli/src/shared/stats.rs @@ -112,7 +112,7 @@ impl Stats { /// Returns the specified percentile for the given data. /// This is best effort since it ignores the interpolation case. fn percentile(mut xs: Vec, p: f64) -> u64 { - xs.sort_unstable(); + xs.sort(); let index = (xs.len() as f64 * p).ceil() as usize - 1; xs[index.clamp(0, xs.len() - 1)] }