From c8e3078ff7a3cff579b84500630e61854355344a Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Thu, 10 Feb 2022 21:13:19 +0000 Subject: [PATCH] Make router benchmarks more realistic by not running test-only code `cargo bench` sets `cfg(test)`, causing us to hit some test-only code in the router when benchmarking, throwing off our benchmarks substantially. Here we swap from the `unstable` feature to a more clearly internal feature (`_bench_unstable`) and also checking for it when enabling test-only code. --- .github/workflows/build.yml | 2 +- lightning-persister/Cargo.toml | 2 +- lightning-persister/src/lib.rs | 6 +++--- lightning/Cargo.toml | 2 +- lightning/src/lib.rs | 4 ++-- lightning/src/ln/channelmanager.rs | 2 +- lightning/src/ln/functional_test_utils.rs | 2 +- lightning/src/routing/network_graph.rs | 2 +- lightning/src/routing/router.rs | 12 ++++++------ 9 files changed, 17 insertions(+), 17 deletions(-) diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index fef042bee..c982060a8 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -231,7 +231,7 @@ jobs: cd .. - name: Run benchmarks on Rust ${{ matrix.toolchain }} run: | - cargo bench --features unstable + cargo bench --features _bench_unstable check_commits: runs-on: ubuntu-latest diff --git a/lightning-persister/Cargo.toml b/lightning-persister/Cargo.toml index 27f772cf3..79ffeeeb2 100644 --- a/lightning-persister/Cargo.toml +++ b/lightning-persister/Cargo.toml @@ -9,7 +9,7 @@ Utilities to manage Rust-Lightning channel data persistence and retrieval. """ [features] -unstable = ["lightning/unstable"] +_bench_unstable = ["lightning/_bench_unstable"] [dependencies] bitcoin = "0.27" diff --git a/lightning-persister/src/lib.rs b/lightning-persister/src/lib.rs index b853b5796..558f4b8fe 100644 --- a/lightning-persister/src/lib.rs +++ b/lightning-persister/src/lib.rs @@ -3,8 +3,8 @@ #![deny(broken_intra_doc_links)] #![deny(missing_docs)] -#![cfg_attr(all(test, feature = "unstable"), feature(test))] -#[cfg(all(test, feature = "unstable"))] extern crate test; +#![cfg_attr(all(test, feature = "_bench_unstable"), feature(test))] +#[cfg(all(test, feature = "_bench_unstable"))] extern crate test; mod util; @@ -362,7 +362,7 @@ mod tests { } } -#[cfg(all(test, feature = "unstable"))] +#[cfg(all(test, feature = "_bench_unstable"))] pub mod bench { use test::Bencher; diff --git a/lightning/Cargo.toml b/lightning/Cargo.toml index 135d42a6a..c5d0c5564 100644 --- a/lightning/Cargo.toml +++ b/lightning/Cargo.toml @@ -24,7 +24,7 @@ max_level_trace = [] # Allow signing of local transactions that may have been revoked or will be revoked, for functional testing (e.g. justice tx handling). # This is unsafe to use in production because it may result in the counterparty publishing taking our funds. unsafe_revoked_tx_signing = [] -unstable = [] +_bench_unstable = [] no-std = ["hashbrown", "bitcoin/no-std", "core2/alloc"] std = ["bitcoin/std"] diff --git a/lightning/src/lib.rs b/lightning/src/lib.rs index ca18d7bb5..8fdf63ffa 100644 --- a/lightning/src/lib.rs +++ b/lightning/src/lib.rs @@ -30,8 +30,8 @@ #![cfg_attr(all(not(feature = "std"), not(test)), no_std)] -#![cfg_attr(all(any(test, feature = "_test_utils"), feature = "unstable"), feature(test))] -#[cfg(all(any(test, feature = "_test_utils"), feature = "unstable"))] extern crate test; +#![cfg_attr(all(any(test, feature = "_test_utils"), feature = "_bench_unstable"), feature(test))] +#[cfg(all(any(test, feature = "_test_utils"), feature = "_bench_unstable"))] extern crate test; #[cfg(not(any(feature = "std", feature = "no-std")))] compile_error!("at least one of the `std` or `no-std` features must be enabled"); diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index 451d6d5f8..2b0e4a0be 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -7132,7 +7132,7 @@ mod tests { } } -#[cfg(all(any(test, feature = "_test_utils"), feature = "unstable"))] +#[cfg(all(any(test, feature = "_test_utils"), feature = "_bench_unstable"))] pub mod bench { use chain::Listen; use chain::chainmonitor::{ChainMonitor, Persist}; diff --git a/lightning/src/ln/functional_test_utils.rs b/lightning/src/ln/functional_test_utils.rs index e8931c1ed..b0ec07947 100644 --- a/lightning/src/ln/functional_test_utils.rs +++ b/lightning/src/ln/functional_test_utils.rs @@ -1137,7 +1137,7 @@ macro_rules! expect_pending_htlcs_forwardable_from_events { }} } -#[cfg(any(test, feature = "unstable"))] +#[cfg(any(test, feature = "_bench_unstable"))] macro_rules! expect_payment_received { ($node: expr, $expected_payment_hash: expr, $expected_payment_secret: expr, $expected_recv_value: expr) => { let events = $node.node.get_and_clear_pending_events(); diff --git a/lightning/src/routing/network_graph.rs b/lightning/src/routing/network_graph.rs index ee310830a..5a84f35c2 100644 --- a/lightning/src/routing/network_graph.rs +++ b/lightning/src/routing/network_graph.rs @@ -2687,7 +2687,7 @@ mod tests { } } -#[cfg(all(test, feature = "unstable"))] +#[cfg(all(test, feature = "_bench_unstable"))] mod benches { use super::*; diff --git a/lightning/src/routing/router.rs b/lightning/src/routing/router.rs index a21cf79a1..dc2f8fd11 100644 --- a/lightning/src/routing/router.rs +++ b/lightning/src/routing/router.rs @@ -455,7 +455,7 @@ struct PathBuildingHop<'a> { /// decrease as well. Thus, we have to explicitly track which nodes have been processed and /// avoid processing them again. was_processed: bool, - #[cfg(any(test, feature = "fuzztarget"))] + #[cfg(all(not(feature = "_bench_unstable"), any(test, feature = "fuzztarget")))] // In tests, we apply further sanity checks on cases where we skip nodes we already processed // to ensure it is specifically in cases where the fee has gone down because of a decrease in // value_contribution_msat, which requires tracking it here. See comments below where it is @@ -896,14 +896,14 @@ where L::Target: Logger { path_htlc_minimum_msat, path_penalty_msat: u64::max_value(), was_processed: false, - #[cfg(any(test, feature = "fuzztarget"))] + #[cfg(all(not(feature = "_bench_unstable"), any(test, feature = "fuzztarget")))] value_contribution_msat, } }); #[allow(unused_mut)] // We only use the mut in cfg(test) let mut should_process = !old_entry.was_processed; - #[cfg(any(test, feature = "fuzztarget"))] + #[cfg(all(not(feature = "_bench_unstable"), any(test, feature = "fuzztarget")))] { // In test/fuzzing builds, we do extra checks to make sure the skipping // of already-seen nodes only happens in cases we expect (see below). @@ -992,13 +992,13 @@ where L::Target: Logger { old_entry.fee_msat = 0; // This value will be later filled with hop_use_fee_msat of the following channel old_entry.path_htlc_minimum_msat = path_htlc_minimum_msat; old_entry.path_penalty_msat = path_penalty_msat; - #[cfg(any(test, feature = "fuzztarget"))] + #[cfg(all(not(feature = "_bench_unstable"), any(test, feature = "fuzztarget")))] { old_entry.value_contribution_msat = value_contribution_msat; } did_add_update_path_to_src_node = true; } else if old_entry.was_processed && new_cost < old_cost { - #[cfg(any(test, feature = "fuzztarget"))] + #[cfg(all(not(feature = "_bench_unstable"), any(test, feature = "fuzztarget")))] { // If we're skipping processing a node which was previously // processed even though we found another path to it with a @@ -4976,7 +4976,7 @@ pub(crate) mod test_utils { } } -#[cfg(all(test, feature = "unstable", not(feature = "no-std")))] +#[cfg(all(test, feature = "_bench_unstable", not(feature = "no-std")))] mod benches { use super::*; use bitcoin::hashes::Hash; -- 2.39.5