Skip to content

Commit

Permalink
fix(nns): Fix benchmark by simulating proper behavior in canbench-rs …
Browse files Browse the repository at this point in the history 
…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.
  • Loading branch information
@max-dfinity
max-dfinity authored Dec 13, 2024
1 parent 3e77cd2 commit ee52ab3
Show file tree
Hide file tree
Showing 2 changed files with 78 additions and 45 deletions.
46 changes: 23 additions & 23 deletions rs/nns/governance/canbench/canbench_results.yml
View file
Original file line number Diff line number Diff line change
@@ -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: {}
Expand All @@ -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: {}
Expand All @@ -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: {}
Expand All @@ -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: {}
Expand All @@ -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: {}
Expand Down
77 changes: 55 additions & 22 deletions rs/nns/governance/src/voting.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -29,43 +29,76 @@ 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<bool> = const { std::cell::Cell::new(false) }
static OVER_SOFT_MESSAGE_LIMIT: std::cell::Cell<bool> = const { std::cell::Cell::new(false) };
static CANBENCH_FAKE_INSTRUCTION_COUNTER: std::cell::Cell<Option<u64>> = const { std::cell::Cell::new(None) };
}

#[cfg(test)]
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<u64>,
_panic_threshold: Option<u64>,
) -> 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<u64>,
) -> 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(
Expand Down

0 comments on commit ee52ab3

Please sign in to comment.