From ee52ab3056cf5f39b09b08de70bdd20485c8b2dc Mon Sep 17 00:00:00 2001 From: max-dfinity <100170574+max-dfinity@users.noreply.github.com> Date: Fri, 13 Dec 2024 13:32:07 -0800 Subject: [PATCH] fix(nns): Fix benchmark by simulating proper behavior in canbench-rs build (#3160) In order to get approximately the same number of read/writes in stable for the VotingStateMachine, it is required to count accurately. Canbench doesn't allow queries, which means the instruction_counter never gives a different result, which means the benchmark thinks it runs out of cycles on every iteration, and thus saves and restores the ProposalVotingStateMachine over and over, leading to an inaccurate benchmark compared to production. This adds code to approximately simulate behavior in canbench-rs build. --- .../governance/canbench/canbench_results.yml | 46 +++++------ rs/nns/governance/src/voting.rs | 77 +++++++++++++------ 2 files changed, 78 insertions(+), 45 deletions(-) diff --git a/rs/nns/governance/canbench/canbench_results.yml b/rs/nns/governance/canbench/canbench_results.yml index b28b55b398e..2fdb55aeccd 100644 --- a/rs/nns/governance/canbench/canbench_results.yml +++ b/rs/nns/governance/canbench/canbench_results.yml @@ -1,73 +1,73 @@ benches: add_neuron_active_maximum: total: - instructions: 36164619 + instructions: 36155076 heap_increase: 1 stable_memory_increase: 0 scopes: {} add_neuron_active_typical: total: - instructions: 1834736 + instructions: 1834230 heap_increase: 0 stable_memory_increase: 0 scopes: {} add_neuron_inactive_maximum: total: - instructions: 96124805 + instructions: 96097938 heap_increase: 1 stable_memory_increase: 0 scopes: {} add_neuron_inactive_typical: total: - instructions: 7369666 + instructions: 7366914 heap_increase: 0 stable_memory_increase: 0 scopes: {} cascading_vote_all_heap: total: - instructions: 34418252 + instructions: 34431941 heap_increase: 0 stable_memory_increase: 128 scopes: {} cascading_vote_heap_neurons_stable_index: total: - instructions: 56553893 + instructions: 56566838 heap_increase: 0 stable_memory_increase: 128 scopes: {} cascading_vote_stable_everything: total: - instructions: 371815016 + instructions: 163131819 heap_increase: 0 stable_memory_increase: 128 scopes: {} cascading_vote_stable_neurons_with_heap_index: total: - instructions: 349629136 + instructions: 140864617 heap_increase: 0 stable_memory_increase: 128 scopes: {} centralized_following_all_stable: total: - instructions: 174310560 + instructions: 68259968 heap_increase: 0 stable_memory_increase: 128 scopes: {} compute_ballots_for_new_proposal_with_stable_neurons: total: - instructions: 1815415 + instructions: 1815411 heap_increase: 0 stable_memory_increase: 0 scopes: {} draw_maturity_from_neurons_fund_heap: total: - instructions: 7242119 + instructions: 7248219 heap_increase: 0 stable_memory_increase: 0 scopes: {} draw_maturity_from_neurons_fund_stable: total: - instructions: 10300850 + instructions: 10293728 heap_increase: 0 stable_memory_increase: 0 scopes: {} @@ -79,13 +79,13 @@ benches: scopes: {} list_active_neurons_fund_neurons_stable: total: - instructions: 2450279 + instructions: 2450275 heap_increase: 0 stable_memory_increase: 0 scopes: {} list_neurons_heap: total: - instructions: 3900350 + instructions: 3900688 heap_increase: 9 stable_memory_increase: 0 scopes: {} @@ -97,13 +97,13 @@ benches: scopes: {} list_neurons_ready_to_unstake_maturity_stable: total: - instructions: 36937657 + instructions: 36937653 heap_increase: 0 stable_memory_increase: 0 scopes: {} list_neurons_stable: total: - instructions: 97878941 + instructions: 97962451 heap_increase: 5 stable_memory_increase: 0 scopes: {} @@ -115,19 +115,19 @@ benches: scopes: {} list_ready_to_spawn_neuron_ids_stable: total: - instructions: 36926358 + instructions: 36926354 heap_increase: 0 stable_memory_increase: 0 scopes: {} neuron_data_validation_heap: total: - instructions: 349808709 + instructions: 349668956 heap_increase: 0 stable_memory_increase: 0 scopes: {} neuron_data_validation_stable: total: - instructions: 312718406 + instructions: 312681996 heap_increase: 0 stable_memory_increase: 0 scopes: {} @@ -139,25 +139,25 @@ benches: scopes: {} neuron_metrics_calculation_stable: total: - instructions: 2476869 + instructions: 2476865 heap_increase: 0 stable_memory_increase: 0 scopes: {} range_neurons_performance: total: - instructions: 47700446 + instructions: 47700442 heap_increase: 0 stable_memory_increase: 0 scopes: {} single_vote_all_stable: total: - instructions: 2477911 + instructions: 2731286 heap_increase: 0 stable_memory_increase: 128 scopes: {} update_recent_ballots_stable_memory: total: - instructions: 236980 + instructions: 236846 heap_increase: 0 stable_memory_increase: 0 scopes: {} diff --git a/rs/nns/governance/src/voting.rs b/rs/nns/governance/src/voting.rs index 7b2ac38d4d4..3c3aa07786e 100644 --- a/rs/nns/governance/src/voting.rs +++ b/rs/nns/governance/src/voting.rs @@ -4,7 +4,7 @@ use crate::{ pb::v1::{Ballot, ProposalData, Topic, Topic::NeuronManagement, Vote}, storage::with_voting_state_machines_mut, }; -#[cfg(not(test))] +#[cfg(not(any(test, feature = "canbench-rs")))] use ic_nervous_system_long_message::is_message_over_threshold; #[cfg(test)] use ic_nervous_system_temporary::Temporary; @@ -29,15 +29,11 @@ const SOFT_VOTING_INSTRUCTIONS_LIMIT: u64 = if cfg!(feature = "test") { BILLION }; -#[cfg(not(test))] -fn over_soft_message_limit() -> bool { - is_message_over_threshold(SOFT_VOTING_INSTRUCTIONS_LIMIT) -} - // The following test methods let us test this internally -#[cfg(test)] +#[cfg(any(test, feature = "canbench-rs"))] thread_local! { - static OVER_SOFT_MESSAGE_LIMIT: std::cell::Cell = const { std::cell::Cell::new(false) } + static OVER_SOFT_MESSAGE_LIMIT: std::cell::Cell = const { std::cell::Cell::new(false) }; + static CANBENCH_FAKE_INSTRUCTION_COUNTER: std::cell::Cell> = const { std::cell::Cell::new(None) }; } #[cfg(test)] @@ -45,27 +41,64 @@ fn temporarily_set_over_soft_message_limit(over: bool) -> Temporary { Temporary::new(&OVER_SOFT_MESSAGE_LIMIT, over) } -#[cfg(test)] +// For tests, we want to switch it on and off atomically. For canbench, we want to +// actually count the instructions. +#[cfg(any(test, feature = "canbench-rs"))] fn over_soft_message_limit() -> bool { - OVER_SOFT_MESSAGE_LIMIT.with(|over| over.get()) + if cfg!(test) { + return OVER_SOFT_MESSAGE_LIMIT.with(|over| over.get()); + } else if cfg!(feature = "canbench-rs") { + return CANBENCH_FAKE_INSTRUCTION_COUNTER.with(|counter| { + let current_instruction_counter = ic_cdk::api::instruction_counter(); + let stored_value = counter.get(); + + if let Some(limit) = stored_value { + current_instruction_counter > limit + } else { + let limit = ic_cdk::api::instruction_counter() + SOFT_VOTING_INSTRUCTIONS_LIMIT; + counter.set(Some(limit)); + false // Since it's the first time, assume not over limit + } + }); + } + // We should not get here + true } +// Production implementation +#[cfg(not(any(test, feature = "canbench-rs")))] +fn over_soft_message_limit() -> bool { + is_message_over_threshold(SOFT_VOTING_INSTRUCTIONS_LIMIT) +} + +// canbench doesn't currently support query calls inside of benchmarks +// We send a no-op message to self to break up the call context into more messages +#[cfg(feature = "canbench-rs")] async fn noop_self_call_if_over_instructions( message_threshold: u64, - panic_threshold: Option, + _panic_threshold: Option, ) -> Result<(), String> { - // canbench doesn't currently support query calls inside of benchmarks - // We send a no-op message to self to break up the call context into more messages - if cfg!(not(feature = "canbench-rs")) { - ic_nervous_system_long_message::noop_self_call_if_over_instructions( - message_threshold, - panic_threshold, - ) - .await - .map_err(|e| e.to_string()) - } else { - Ok(()) + if over_soft_message_limit() { + let limit = ic_cdk::api::instruction_counter() + message_threshold; + CANBENCH_FAKE_INSTRUCTION_COUNTER.with(|counter| { + counter.set(Some(limit)); + }); } + Ok(()) +} + +// Production implementation +#[cfg(not(feature = "canbench-rs"))] +async fn noop_self_call_if_over_instructions( + message_threshold: u64, + panic_threshold: Option, +) -> Result<(), String> { + ic_nervous_system_long_message::noop_self_call_if_over_instructions( + message_threshold, + panic_threshold, + ) + .await + .map_err(|e| e.to_string()) } fn proposal_ballots(