Skip to content

Commit

Permalink
fix: Account instructions for bytes accessed and dirty pages on syste…
Browse files Browse the repository at this point in the history 
…m subnets (#3396)

This is another change that aims to make system subnets behave closer to
application subnets.
  • Loading branch information
@dsarlis
dsarlis authored Jan 13, 2025
1 parent 92ae5bc commit 4e0a28d
Show file tree
Hide file tree
Showing 4 changed files with 78 additions and 126 deletions.
3 changes: 1 addition & 2 deletions rs/config/src/subnet_config.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -140,7 +140,6 @@ const DEFAULT_REFERENCE_SUBNET_SIZE: usize = 13;

/// Costs for each newly created dirty page in stable memory.
const DEFAULT_DIRTY_PAGE_OVERHEAD: NumInstructions = NumInstructions::new(1_000);
const SYSTEM_SUBNET_DIRTY_PAGE_OVERHEAD: NumInstructions = NumInstructions::new(0);

/// Accumulated priority reset interval, rounds.
///
Expand Down Expand Up @@ -341,7 +340,7 @@ impl SchedulerConfig {
// This limit should be high enough (1000T) to effectively disable
// rate-limiting for the system subnets.
install_code_rate_limit: NumInstructions::from(1_000_000_000_000_000),
dirty_page_overhead: SYSTEM_SUBNET_DIRTY_PAGE_OVERHEAD,
dirty_page_overhead: DEFAULT_DIRTY_PAGE_OVERHEAD,
accumulated_priority_reset_interval: ACCUMULATED_PRIORITY_RESET_INTERVAL,
upload_wasm_chunk_instructions: NumInstructions::from(0),
canister_snapshot_baseline_instructions: NumInstructions::from(0),
Expand Down
184 changes: 74 additions & 110 deletions rs/embedders/tests/wasmtime_random_memory_writes.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -980,126 +980,90 @@ mod tests {

#[test]
fn test_proportional_instructions_consumption_to_data_size() {
with_test_replica_logger(|log| {
let subnet_type = SubnetType::Application;
let dst: u32 = 0;

let dirty_heap_cost = match EmbeddersConfig::default().metering_type {
ic_config::embedders::MeteringType::New => SchedulerConfig::application_subnet()
.dirty_page_overhead
.get(),
_ => 0,
};
for subnet_type in [
SubnetType::Application,
SubnetType::VerifiedApplication,
SubnetType::System,
] {
with_test_replica_logger(|log| {
let dst: u32 = 0;

let dirty_heap_cost = match EmbeddersConfig::default().metering_type {
ic_config::embedders::MeteringType::New => match subnet_type {
SubnetType::System => {
SchedulerConfig::system_subnet().dirty_page_overhead.get()
}
SubnetType::Application => SchedulerConfig::application_subnet()
.dirty_page_overhead
.get(),
SubnetType::VerifiedApplication => {
SchedulerConfig::verified_application_subnet()
.dirty_page_overhead
.get()
}
},
_ => 0,
};

let mut payload: Vec<u8> = dst.to_le_bytes().to_vec();
payload.extend(random_payload());
let payload_size = payload.len() - 4;
let mut payload: Vec<u8> = dst.to_le_bytes().to_vec();
payload.extend(random_payload());
let payload_size = payload.len() - 4;

let mut double_size_payload: Vec<u8> = payload.clone();
double_size_payload.extend(random_payload());
let mut double_size_payload: Vec<u8> = payload.clone();
double_size_payload.extend(random_payload());

let (instructions_consumed_without_data, dry_run_stats) = run_and_get_stats(
log.clone(),
"write_bytes",
dst.to_le_bytes().to_vec(),
MAX_NUM_INSTRUCTIONS,
subnet_type,
)
.unwrap();
let dry_run_dirty_heap = dry_run_stats.wasm_dirty_pages.len() as u64;

{
// Number of instructions consumed only for copying the payload.
let (consumed_instructions, run_stats) = run_and_get_stats(
let (instructions_consumed_without_data, dry_run_stats) = run_and_get_stats(
log.clone(),
"write_bytes",
payload,
MAX_NUM_INSTRUCTIONS,
subnet_type,
)
.unwrap();
let dirty_heap = run_stats.wasm_dirty_pages.len() as u64;
let consumed_instructions =
consumed_instructions - instructions_consumed_without_data;
assert_eq!(
(consumed_instructions.get() - dirty_heap * dirty_heap_cost) as usize,
(payload_size / BYTES_PER_INSTRUCTION)
- (dry_run_dirty_heap * dirty_heap_cost) as usize,
);
}

{
// Number of instructions consumed increased with the size of the data.
let (consumed_instructions, run_stats) = run_and_get_stats(
log,
"write_bytes",
double_size_payload,
dst.to_le_bytes().to_vec(),
MAX_NUM_INSTRUCTIONS,
subnet_type,
)
.unwrap();
let dirty_heap = run_stats.wasm_dirty_pages.len() as u64;
let consumed_instructions =
consumed_instructions - instructions_consumed_without_data;

assert_eq!(
(consumed_instructions.get() - dirty_heap * dirty_heap_cost) as usize,
(2 * payload_size / BYTES_PER_INSTRUCTION)
- (dry_run_dirty_heap * dirty_heap_cost) as usize
);
}
})
}

#[test]
fn test_no_instructions_consumption_based_on_data_size_on_system_subnet() {
with_test_replica_logger(|log| {
let subnet_type = SubnetType::System;
let dst: u32 = 0;

let mut payload: Vec<u8> = dst.to_le_bytes().to_vec();
payload.extend(random_payload());

let mut double_size_payload: Vec<u8> = payload.clone();
double_size_payload.extend(random_payload());

let instructions_consumed_without_data = get_num_instructions_consumed(
log.clone(),
"write_bytes",
dst.to_le_bytes().to_vec(),
MAX_NUM_INSTRUCTIONS,
subnet_type,
)
.unwrap();

{
// Number of instructions consumed for copying the payload is zero.
let consumed_instructions = get_num_instructions_consumed(
log.clone(),
"write_bytes",
payload,
MAX_NUM_INSTRUCTIONS,
subnet_type,
)
.unwrap()
- instructions_consumed_without_data;
assert_eq!(consumed_instructions.get(), 0);
}
let dry_run_dirty_heap = dry_run_stats.wasm_dirty_pages.len() as u64;

{
// Number of instructions consumed only for copying the payload.
let (consumed_instructions, run_stats) = run_and_get_stats(
log.clone(),
"write_bytes",
payload,
MAX_NUM_INSTRUCTIONS,
subnet_type,
)
.unwrap();
let dirty_heap = run_stats.wasm_dirty_pages.len() as u64;
let consumed_instructions =
consumed_instructions - instructions_consumed_without_data;
assert_eq!(
(consumed_instructions.get() - dirty_heap * dirty_heap_cost) as usize,
(payload_size / BYTES_PER_INSTRUCTION)
- (dry_run_dirty_heap * dirty_heap_cost) as usize,
);
}

{
// Number of instructions consumed for copying the payload is zero.
let consumed_instructions = get_num_instructions_consumed(
log,
"write_bytes",
double_size_payload,
MAX_NUM_INSTRUCTIONS,
subnet_type,
)
.unwrap()
- instructions_consumed_without_data;
assert_eq!(consumed_instructions.get(), 0);
}
})
{
// Number of instructions consumed increased with the size of the data.
let (consumed_instructions, run_stats) = run_and_get_stats(
log,
"write_bytes",
double_size_payload,
MAX_NUM_INSTRUCTIONS,
subnet_type,
)
.unwrap();
let dirty_heap = run_stats.wasm_dirty_pages.len() as u64;
let consumed_instructions =
consumed_instructions - instructions_consumed_without_data;

assert_eq!(
(consumed_instructions.get() - dirty_heap * dirty_heap_cost) as usize,
(2 * payload_size / BYTES_PER_INSTRUCTION)
- (dry_run_dirty_heap * dirty_heap_cost) as usize
);
}
})
}
}

fn run_and_get_stats(
Expand Down
10 changes: 2 additions & 8 deletions rs/execution_environment/src/execution/update/tests.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ use std::time::Duration;
use assert_matches::assert_matches;

use ic_base_types::NumSeconds;
use ic_config::subnet_config::SchedulerConfig;
use ic_error_types::ErrorCode;
use ic_interfaces::execution_environment::SubnetAvailableMemory;
use ic_registry_subnet_type::SubnetType;
Expand Down Expand Up @@ -352,7 +351,7 @@ fn dts_update_concurrent_cycles_change_fails() {
}

#[test]
fn dirty_pages_are_free_on_system_subnet() {
fn dirty_pages_cost_the_same_on_app_and_system_subnets() {
fn instructions_to_write_stable_byte(mut test: ExecutionTest) -> NumInstructions {
let initial_cycles = Cycles::new(1_000_000_000_000);
let a_id = test.universal_canister_with_cycles(initial_cycles).unwrap();
Expand All @@ -376,12 +375,7 @@ fn dirty_pages_are_free_on_system_subnet() {
.build();
let app_instructions = instructions_to_write_stable_byte(app_test);

// Can't check for equality because there are other charges that are omitted
// on system subnets.
assert!(
app_instructions
> system_instructions + SchedulerConfig::application_subnet().dirty_page_overhead
);
assert_eq!(app_instructions, system_instructions);
}

#[test]
Expand Down
7 changes: 1 addition & 6 deletions rs/system_api/src/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1639,12 +1639,7 @@ impl SystemApi for SystemApiImpl {
}

fn get_num_instructions_from_bytes(&self, num_bytes: NumBytes) -> NumInstructions {
match self.sandbox_safe_system_state.subnet_type {
SubnetType::System => NumInstructions::from(0),
SubnetType::VerifiedApplication | SubnetType::Application => {
NumInstructions::from(num_bytes.get())
}
}
NumInstructions::from(num_bytes.get())
}

fn stable_memory_dirty_pages(&self) -> Vec<(PageIndex, &PageBytes)> {
Expand Down

0 comments on commit 4e0a28d

Please sign in to comment.