From e2156c06c47150149a227d1d05a55c406dc3d59c Mon Sep 17 00:00:00 2001 From: "Tobin C. Harding" Date: Wed, 30 Nov 2022 09:18:43 +1100 Subject: [PATCH] Use RUSTFLAGS instead of feature to gate benching Currently we use the `"_bench_unstable"` feature to gate benching code. This results in features not being additive for stable toolchains i.e., one cannot currently run `cargo +stable check --all-features`. Although this method of gating bench code is widely proposed as a good solution online there is another solution that keeps all features additive. Guard the bench code with `#[cfg(bench)]` and when running benches use `RUSTFLAGS='--cfg=bench'` to turn on `bench`. --- .github/workflows/build.yml | 3 ++- CONTRIBUTING.md | 8 ++++++++ lightning-persister/Cargo.toml | 1 - lightning-persister/src/lib.rs | 6 +++--- lightning-rapid-gossip-sync/Cargo.toml | 1 - lightning-rapid-gossip-sync/src/lib.rs | 8 +++----- lightning/Cargo.toml | 1 - lightning/src/lib.rs | 6 +++--- lightning/src/ln/channelmanager.rs | 2 +- lightning/src/ln/functional_test_utils.rs | 8 ++++---- lightning/src/routing/gossip.rs | 2 +- lightning/src/routing/router.rs | 14 +++++++------- lightning/src/sign/mod.rs | 2 +- lightning/src/sync/mod.rs | 14 +++++++------- 14 files changed, 40 insertions(+), 36 deletions(-) diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index 8e02ec9da75..62fd23fc11b 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -141,7 +141,8 @@ jobs: cd .. - name: Run benchmarks on Rust ${{ matrix.toolchain }} run: | - RUSTC_BOOTSTRAP=1 cargo bench --features _bench_unstable + + RUSTC_BOOTSTRAP=1 RUSTFLAGS='--cfg=bench' cargo bench check_commits: runs-on: ubuntu-latest diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index e795ecb9fba..b65b19084b0 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -167,6 +167,14 @@ Fuzzing is heavily encouraged: you will find all related material under `fuzz/` Mutation testing is work-in-progress; any contribution there would be warmly welcomed. +Benchmarks +---------- + +We use a custom Rust compiler configuration conditional to guard the bench mark +code. To run the bench marks use `RUSTFLAGS='--cfg=bench' cargo +nightly bench`. + + + C/C++ Bindings -------------- diff --git a/lightning-persister/Cargo.toml b/lightning-persister/Cargo.toml index 88132bdcfd6..2d0fcca0285 100644 --- a/lightning-persister/Cargo.toml +++ b/lightning-persister/Cargo.toml @@ -14,7 +14,6 @@ all-features = true rustdoc-args = ["--cfg", "docsrs"] [features] -_bench_unstable = ["lightning/_bench_unstable"] [dependencies] bitcoin = "0.29.0" diff --git a/lightning-persister/src/lib.rs b/lightning-persister/src/lib.rs index d25ab6f9fca..c3d5e4ba5ae 100644 --- a/lightning-persister/src/lib.rs +++ b/lightning-persister/src/lib.rs @@ -8,8 +8,8 @@ #![cfg_attr(docsrs, feature(doc_auto_cfg))] -#![cfg_attr(all(test, feature = "_bench_unstable"), feature(test))] -#[cfg(all(test, feature = "_bench_unstable"))] extern crate test; +#![cfg_attr(bench, feature(test))] +#[cfg(bench)] extern crate test; mod util; @@ -338,7 +338,7 @@ mod tests { } } -#[cfg(all(test, feature = "_bench_unstable"))] +#[cfg(bench)] pub mod bench { use test::Bencher; diff --git a/lightning-rapid-gossip-sync/Cargo.toml b/lightning-rapid-gossip-sync/Cargo.toml index 6673431d93b..c07182df623 100644 --- a/lightning-rapid-gossip-sync/Cargo.toml +++ b/lightning-rapid-gossip-sync/Cargo.toml @@ -13,7 +13,6 @@ Utility to process gossip routing data from Rapid Gossip Sync Server. default = ["std"] no-std = ["lightning/no-std"] std = ["lightning/std"] -_bench_unstable = [] [dependencies] lightning = { version = "0.0.115", path = "../lightning", default-features = false } diff --git a/lightning-rapid-gossip-sync/src/lib.rs b/lightning-rapid-gossip-sync/src/lib.rs index c8f140cc189..9e5693c0f66 100644 --- a/lightning-rapid-gossip-sync/src/lib.rs +++ b/lightning-rapid-gossip-sync/src/lib.rs @@ -64,10 +64,8 @@ #![cfg_attr(all(not(feature = "std"), not(test)), no_std)] -// Allow and import test features for benching -#![cfg_attr(all(test, feature = "_bench_unstable"), feature(test))] -#[cfg(all(test, feature = "_bench_unstable"))] -extern crate test; +#![cfg_attr(bench, feature(test))] +#[cfg(bench)] extern crate test; #[cfg(not(feature = "std"))] extern crate alloc; @@ -287,7 +285,7 @@ mod tests { } } -#[cfg(all(test, feature = "_bench_unstable"))] +#[cfg(bench)] pub mod bench { use test::Bencher; diff --git a/lightning/Cargo.toml b/lightning/Cargo.toml index 1804be0f1bb..3c9c32f27ab 100644 --- a/lightning/Cargo.toml +++ b/lightning/Cargo.toml @@ -28,7 +28,6 @@ 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 = [] -_bench_unstable = [] # Override signing to not include randomness when generating signatures for test vectors. _test_vectors = [] diff --git a/lightning/src/lib.rs b/lightning/src/lib.rs index e7e7e0ede6b..054feb4b3fb 100644 --- a/lightning/src/lib.rs +++ b/lightning/src/lib.rs @@ -54,8 +54,8 @@ #![cfg_attr(all(not(feature = "std"), not(test)), no_std)] -#![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_attr(bench, feature(test))] +#[cfg(bench)] 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"); @@ -177,7 +177,7 @@ mod prelude { pub use alloc::string::ToString; } -#[cfg(all(not(feature = "_bench_unstable"), feature = "backtrace", feature = "std", test))] +#[cfg(all(not(bench), feature = "backtrace", feature = "std", test))] extern crate backtrace; mod sync; diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index fb413b22f24..8fff5d3bee3 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -9266,7 +9266,7 @@ mod tests { } } -#[cfg(all(any(test, feature = "_test_utils"), feature = "_bench_unstable"))] +#[cfg(bench)] pub mod bench { use crate::chain::Listen; use crate::chain::chainmonitor::{ChainMonitor, Persist}; diff --git a/lightning/src/ln/functional_test_utils.rs b/lightning/src/ln/functional_test_utils.rs index a75d0c92285..7d6c7baf10c 100644 --- a/lightning/src/ln/functional_test_utils.rs +++ b/lightning/src/ln/functional_test_utils.rs @@ -1769,7 +1769,7 @@ macro_rules! get_route_and_payment_hash { } #[macro_export] -#[cfg(any(test, feature = "_bench_unstable", feature = "_test_utils"))] +#[cfg(any(test, bench, feature = "_test_utils"))] macro_rules! expect_payment_claimable { ($node: expr, $expected_payment_hash: expr, $expected_payment_secret: expr, $expected_recv_value: expr) => { expect_payment_claimable!($node, $expected_payment_hash, $expected_payment_secret, $expected_recv_value, None, $node.node.get_our_node_id()) @@ -1796,7 +1796,7 @@ macro_rules! expect_payment_claimable { } #[macro_export] -#[cfg(any(test, feature = "_bench_unstable", feature = "_test_utils"))] +#[cfg(any(test, bench, feature = "_test_utils"))] macro_rules! expect_payment_claimed { ($node: expr, $expected_payment_hash: expr, $expected_recv_value: expr) => { let events = $node.node.get_and_clear_pending_events(); @@ -1913,7 +1913,7 @@ macro_rules! expect_payment_forwarded { } } -#[cfg(any(test, feature = "_bench_unstable", feature = "_test_utils"))] +#[cfg(any(test, bench, feature = "_test_utils"))] pub fn expect_channel_pending_event<'a, 'b, 'c, 'd>(node: &'a Node<'b, 'c, 'd>, expected_counterparty_node_id: &PublicKey) { let events = node.node.get_and_clear_pending_events(); assert_eq!(events.len(), 1); @@ -1925,7 +1925,7 @@ pub fn expect_channel_pending_event<'a, 'b, 'c, 'd>(node: &'a Node<'b, 'c, 'd>, } } -#[cfg(any(test, feature = "_bench_unstable", feature = "_test_utils"))] +#[cfg(any(test, bench, feature = "_test_utils"))] pub fn expect_channel_ready_event<'a, 'b, 'c, 'd>(node: &'a Node<'b, 'c, 'd>, expected_counterparty_node_id: &PublicKey) { let events = node.node.get_and_clear_pending_events(); assert_eq!(events.len(), 1); diff --git a/lightning/src/routing/gossip.rs b/lightning/src/routing/gossip.rs index c5c08cf4032..1d09598a699 100644 --- a/lightning/src/routing/gossip.rs +++ b/lightning/src/routing/gossip.rs @@ -3388,7 +3388,7 @@ pub(crate) mod tests { } } -#[cfg(all(test, feature = "_bench_unstable"))] +#[cfg(bench)] mod benches { use super::*; diff --git a/lightning/src/routing/router.rs b/lightning/src/routing/router.rs index b33e021ab4f..c9c096c8df1 100644 --- a/lightning/src/routing/router.rs +++ b/lightning/src/routing/router.rs @@ -1015,7 +1015,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(all(not(feature = "_bench_unstable"), any(test, fuzzing)))] + #[cfg(all(not(bench), any(test, fuzzing)))] // 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 @@ -1036,7 +1036,7 @@ impl<'a> core::fmt::Debug for PathBuildingHop<'a> { .field("path_penalty_msat", &self.path_penalty_msat) .field("path_htlc_minimum_msat", &self.path_htlc_minimum_msat) .field("cltv_expiry_delta", &self.candidate.cltv_expiry_delta()); - #[cfg(all(not(feature = "_bench_unstable"), any(test, fuzzing)))] + #[cfg(all(not(bench), any(test, fuzzing)))] let debug_struct = debug_struct .field("value_contribution_msat", &self.value_contribution_msat); debug_struct.finish() @@ -1570,14 +1570,14 @@ where L::Target: Logger { path_htlc_minimum_msat, path_penalty_msat: u64::max_value(), was_processed: false, - #[cfg(all(not(feature = "_bench_unstable"), any(test, fuzzing)))] + #[cfg(all(not(bench), any(test, fuzzing)))] value_contribution_msat, } }); #[allow(unused_mut)] // We only use the mut in cfg(test) let mut should_process = !old_entry.was_processed; - #[cfg(all(not(feature = "_bench_unstable"), any(test, fuzzing)))] + #[cfg(all(not(bench), any(test, fuzzing)))] { // 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). @@ -1648,13 +1648,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(all(not(feature = "_bench_unstable"), any(test, fuzzing)))] + #[cfg(all(not(bench), any(test, fuzzing)))] { 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(all(not(feature = "_bench_unstable"), any(test, fuzzing)))] + #[cfg(all(not(bench), any(test, fuzzing)))] { // If we're skipping processing a node which was previously // processed even though we found another path to it with a @@ -6079,7 +6079,7 @@ pub(crate) mod bench_utils { } } -#[cfg(all(test, feature = "_bench_unstable", not(feature = "no-std")))] +#[cfg(all(bench, not(features = "no-std")))] mod benches { use super::*; use bitcoin::hashes::Hash; diff --git a/lightning/src/sign/mod.rs b/lightning/src/sign/mod.rs index cd898f12b32..5342222623f 100644 --- a/lightning/src/sign/mod.rs +++ b/lightning/src/sign/mod.rs @@ -1628,7 +1628,7 @@ pub fn dyn_sign() { let _signer: Box; } -#[cfg(all(test, feature = "_bench_unstable", not(feature = "no-std")))] +#[cfg(all(test, bench, not(feature = "no-std")))] mod benches { use std::sync::{Arc, mpsc}; use std::sync::mpsc::TryRecvError; diff --git a/lightning/src/sync/mod.rs b/lightning/src/sync/mod.rs index 1b2b9a739b8..07a7bc0996b 100644 --- a/lightning/src/sync/mod.rs +++ b/lightning/src/sync/mod.rs @@ -3,7 +3,7 @@ pub(crate) enum LockHeldState { HeldByThread, NotHeldByThread, - #[cfg(any(feature = "_bench_unstable", not(test)))] + #[cfg(any(bench, not(test)))] Unsupported, } @@ -20,20 +20,20 @@ pub(crate) trait LockTestExt<'a> { fn unsafe_well_ordered_double_lock_self(&'a self) -> Self::ExclLock; } -#[cfg(all(feature = "std", not(feature = "_bench_unstable"), test))] +#[cfg(all(feature = "std", not(bench), test))] mod debug_sync; -#[cfg(all(feature = "std", not(feature = "_bench_unstable"), test))] +#[cfg(all(feature = "std", not(bench), test))] pub use debug_sync::*; -#[cfg(all(feature = "std", not(feature = "_bench_unstable"), test))] +#[cfg(all(feature = "std", not(bench), test))] // Note that to make debug_sync's regex work this must not contain `debug_string` in the module name mod test_lockorder_checks; -#[cfg(all(feature = "std", any(feature = "_bench_unstable", not(test))))] +#[cfg(all(feature = "std", any(bench, not(test))))] pub(crate) mod fairrwlock; -#[cfg(all(feature = "std", any(feature = "_bench_unstable", not(test))))] +#[cfg(all(feature = "std", any(bench, not(test))))] pub use {std::sync::{Arc, Mutex, Condvar, MutexGuard, RwLock, RwLockReadGuard, RwLockWriteGuard}, fairrwlock::FairRwLock}; -#[cfg(all(feature = "std", any(feature = "_bench_unstable", not(test))))] +#[cfg(all(feature = "std", any(bench, not(test))))] mod ext_impl { use super::*; impl<'a, T: 'a> LockTestExt<'a> for Mutex {