Skip to content

Commit

Permalink
Merge branch 'maksym/follow-1160-k-data' into 'master'
Browse files Browse the repository at this point in the history
fix: add instructions-per-byte factor to data sending api calls

This MR adds instructions-per-byte conversion factor for charging instructions in data sending api calls to account for sandbox-replica communication.

Specifically:
- `msg_reply_data_append`
- `msg_reject`
- `call_data_append`

When charging instructions for a payload size it's important to account not only for the memory, but also for the time that is spent for transferring the payload from canister's wasm through sandbox into replica. 

See merge request dfinity-lab/public/ic!15315
  • Loading branch information
maksymar committed Oct 30, 2023
2 parents 7dbe6dc + 2c48018 commit 8ec3e3e
Show file tree
Hide file tree
Showing 3 changed files with 13 additions and 7 deletions.
11 changes: 8 additions & 3 deletions rs/embedders/src/wasmtime_embedder/system_api.rs
Expand Up @@ -24,6 +24,11 @@ use std::convert::TryFrom;
use crate::wasmtime_embedder::system_api_complexity::system_api;
use ic_system_api::SystemApiImpl;

/// The amount of instructions required to process a single byte in a payload.
/// This includes the cost of memory as well as time passing the payload
/// from wasm sandbox to the replica execution environment.
const INSTRUCTIONS_PER_BYTE_CONVERSION_FACTOR: u32 = 50;

fn unexpected_err(s: String) -> HypervisorError {
HypervisorError::WasmEngineError(WasmEngineError::Unexpected(s))
}
Expand Down Expand Up @@ -506,7 +511,7 @@ pub(crate) fn syscalls(
charge_for_cpu_and_mem(
&mut caller,
overhead!(MSG_REPLY_DATA_APPEND, metering_type),
size as u64,
(INSTRUCTIONS_PER_BYTE_CONVERSION_FACTOR * size) as u64,
)?;
with_memory_and_system_api(&mut caller, |system_api, memory| {
system_api.ic0_msg_reply_data_append(src, size, memory)
Expand Down Expand Up @@ -539,7 +544,7 @@ pub(crate) fn syscalls(
charge_for_cpu_and_mem(
&mut caller,
overhead!(MSG_REJECT, metering_type),
size as u64,
(INSTRUCTIONS_PER_BYTE_CONVERSION_FACTOR * size) as u64,
)?;
with_memory_and_system_api(&mut caller, |system_api, memory| {
system_api.ic0_msg_reject(src, size, memory)
Expand Down Expand Up @@ -691,7 +696,7 @@ pub(crate) fn syscalls(
charge_for_cpu_and_mem(
&mut caller,
overhead!(CALL_DATA_APPEND, metering_type),
size as u64,
(INSTRUCTIONS_PER_BYTE_CONVERSION_FACTOR * size) as u64,
)?;
with_memory_and_system_api(&mut caller, |system_api, memory| {
system_api.ic0_call_data_append(src, size, memory)
Expand Down
2 changes: 1 addition & 1 deletion rs/execution_environment/src/query_handler/tests.rs
Expand Up @@ -2255,7 +2255,7 @@ fn query_stats_are_collected() {
// instructions and payload sizes differ. All child canisters have the same cost though.
assert_eq!(
canister_query_stats.num_instructions,
if idx == 0 { 69984 } else { 13635 }
if idx == 0 { 72532 } else { 13929 }
);
assert_eq!(
canister_query_stats.ingress_payload_size,
Expand Down
7 changes: 4 additions & 3 deletions rs/execution_environment/tests/dts.rs
Expand Up @@ -502,7 +502,7 @@ fn dts_pending_upgrade_with_heartbeat() {

let env = dts_env(
NumInstructions::from(1_000_000_000),
NumInstructions::from(10_000),
NumInstructions::from(30_000),
);

let binary = wat2wasm(DTS_WAT);
Expand Down Expand Up @@ -1041,9 +1041,10 @@ fn dts_long_running_install_and_update() {
return;
}

let slice_instruction_limit = 15_000_000;
let env = dts_env(
NumInstructions::from(100_000_000),
NumInstructions::from(1_000_000),
NumInstructions::from(slice_instruction_limit),
);

let user_id = PrincipalId::new_anonymous();
Expand Down Expand Up @@ -1121,7 +1122,7 @@ fn dts_long_running_install_and_update() {

for i in 0..30 {
let work = wasm()
.instruction_counter_is_at_least(1_000_000)
.instruction_counter_is_at_least(slice_instruction_limit)
.message_payload()
.append_and_reply()
.build();
Expand Down

0 comments on commit 8ec3e3e

Please sign in to comment.