Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

wip on metering host fns #100

Merged
merged 8 commits into from
Nov 12, 2023
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion crates/host/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@ holochain_serialized_bytes = "=0.0.53"
serde = "1"
tracing = "0.1"
parking_lot = "0.12"
once_cell = "1"
rand = "0.8"
bimap = "0.6"
bytes = "1"
Expand Down
84 changes: 84 additions & 0 deletions crates/host/src/env.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,15 +2,19 @@ use std::num::TryFromIntError;

use crate::guest::read_bytes;
use crate::prelude::*;
use wasmer::Global;
use wasmer::Memory;
use wasmer::StoreMut;
use wasmer::TypedFunction;
use wasmer_middlewares::metering::MeteringPoints;

#[derive(Clone, Default)]
pub struct Env {
pub memory: Option<Memory>,
pub allocate: Option<TypedFunction<i32, i32>>,
pub deallocate: Option<TypedFunction<(i32, i32), ()>>,
pub wasmer_metering_points_exhausted: Option<Global>,
jost-s marked this conversation as resolved.
Show resolved Hide resolved
pub wasmer_metering_remaining_points: Option<Global>,
}

impl Env {
Expand Down Expand Up @@ -97,4 +101,84 @@ impl Env {
}
}
}

/// Mimics upstream function of the same name but accesses the global directly from env.
/// https://github.com/wasmerio/wasmer/blob/master/lib/middlewares/src/metering.rs#L285
pub fn get_remaining_points(
&self,
store_mut: &mut StoreMut,
) -> Result<MeteringPoints, wasmer::RuntimeError> {
let exhaused: i32 = self
thedavidmeister marked this conversation as resolved.
Show resolved Hide resolved
.wasmer_metering_points_exhausted
.as_ref()
.ok_or(wasm_error!(WasmErrorInner::Memory))?
.get(store_mut)
.try_into()
.map_err(|_| wasm_error!(WasmErrorInner::PointerMap))?;

if exhaused > 0 {
return Ok(MeteringPoints::Exhausted);
}

let points = self
.wasmer_metering_remaining_points
.as_ref()
.ok_or(wasm_error!(WasmErrorInner::Memory))?
.get(store_mut)
.try_into()
.map_err(|_| wasm_error!(WasmErrorInner::PointerMap))?;

Ok(MeteringPoints::Remaining(points))
}

pub fn set_remaining_points(
&self,
store_mut: &mut StoreMut,
points: u64,
) -> Result<(), wasmer::RuntimeError> {
self.wasmer_metering_remaining_points
.as_ref()
.ok_or(wasm_error!(WasmErrorInner::Memory))?
.set(store_mut, points.into())
.map_err(|_| wasm_error!(WasmErrorInner::PointerMap))?;

self.wasmer_metering_points_exhausted
.as_ref()
.ok_or(wasm_error!(WasmErrorInner::Memory))?
.set(store_mut, 0i32.into())
.map_err(|_| wasm_error!(WasmErrorInner::PointerMap))?;
Ok(())
}

pub fn decrease_points(
&self,
store_mut: &mut StoreMut,
points: u64,
) -> Result<MeteringPoints, wasmer::RuntimeError> {
match self.get_remaining_points(store_mut) {
Ok(MeteringPoints::Remaining(remaining)) => {
if remaining < points {
self.wasmer_metering_remaining_points
.as_ref()
.ok_or(wasm_error!(WasmErrorInner::Memory))?
.set(store_mut, 0i32.into())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How do these points get returned to the pool? It seems like not keeping part of the capacity requested to be claimed would mean that returning capacity on completion could get out of sync?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like that's not the idea. You allocate a certain amount of capacity and when it's gone it's gone until something calls set to top it back up?

I still wonder if accurately tracking the value to account for overrun and making the 'set' an 'increment by' makes sense?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ThetaSinner well changing the middleware from decreasing points to increasing would mean rewriting the middleware from the ground up

we're using the default setup from wasmer, and what i'm doing here is exposing it so that it can be accessed by host functions as well as the default wasm op counter

Copy link
Member

@ThetaSinner ThetaSinner Nov 8, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't mean dropping the increasing part, the increasing part is fine. I'm looking at set_remaining_points and wondering if that should be a paired up increment rather than a set?

If this is just exposing something wasmer already does then fine

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ThetaSinner it's a direct port of https://github.com/wasmerio/wasmer/blob/master/lib/middlewares/src/metering.rs#L335 but not requiring the instance itself to be in the environment, all we need is the global

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Understood thanks!

.map_err(|_| wasm_error!(WasmErrorInner::PointerMap))?;
self.wasmer_metering_points_exhausted
.as_ref()
.ok_or(wasm_error!(WasmErrorInner::Memory))?
.set(store_mut, 1i32.into())
.map_err(|_| wasm_error!(WasmErrorInner::PointerMap))?;
Ok(MeteringPoints::Exhausted)
} else {
self.wasmer_metering_remaining_points
.as_ref()
.ok_or(wasm_error!(WasmErrorInner::Memory))?
.set(store_mut, (remaining - points).into())
.map_err(|_| wasm_error!(WasmErrorInner::PointerMap))?;
Ok(MeteringPoints::Remaining(remaining - points))
}
}
v => v,
}
}
}
6 changes: 0 additions & 6 deletions crates/host/src/module.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,7 @@ use crate::prelude::*;
use bimap::BiMap;
use bytes::Bytes;
use holochain_wasmer_common::WasmError;
use once_cell::sync::{Lazy, OnceCell};
use parking_lot::Mutex;
use parking_lot::RwLock;
use std::collections::BTreeMap;
use std::sync::Arc;
use wasmer::Cranelift;
Expand Down Expand Up @@ -123,8 +121,6 @@ pub struct SerializedModuleCache {
cranelift: fn() -> Cranelift,
}

pub static SERIALIZED_MODULE_CACHE: OnceCell<RwLock<SerializedModuleCache>> = OnceCell::new();

impl PlruCache for SerializedModuleCache {
type Item = SerializedModule;

Expand Down Expand Up @@ -214,8 +210,6 @@ pub struct InstanceCache {
key_map: PlruKeyMap,
cache: BTreeMap<CacheKey, Arc<InstanceWithStore>>,
}
pub static INSTANCE_CACHE: Lazy<RwLock<InstanceCache>> =
Lazy::new(|| RwLock::new(InstanceCache::default()));

impl PlruCache for InstanceCache {
type Item = InstanceWithStore;
Expand Down
2 changes: 1 addition & 1 deletion test/Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions test/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ parking_lot = "0.12"
wasmer = "=4.2.2"
wasmer-middlewares = "=4.2.2"
test-fuzz = "=3.0.4"
once_cell = "1"

[dev-dependencies]
env_logger = "0.8"
Expand Down
6 changes: 3 additions & 3 deletions test/benches/bench.rs
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,7 @@ pub fn wasm_call(c: &mut Criterion) {
pub fn wasm_call_n(c: &mut Criterion) {
let mut group = c.benchmark_group("wasm_call_n");

let instance_with_store = TestWasm::Io.instance();
let instance_with_store = TestWasm::Io.unmetered_instance();

macro_rules! bench_n {
( $fs:expr; $t:ty; ) => {
Expand Down Expand Up @@ -165,7 +165,7 @@ pub fn wasm_call_n(c: &mut Criterion) {
pub fn test_process_string(c: &mut Criterion) {
let mut group = c.benchmark_group("test_process_string");

let instance_with_store = TestWasm::Test.instance();
let instance_with_store = TestWasm::Test.unmetered_instance();

for n in vec![0, 1, 1_000, 1_000_000] {
group.throughput(Throughput::Bytes(n));
Expand Down Expand Up @@ -203,7 +203,7 @@ pub fn test_instances(c: &mut Criterion) {
let mut jhs = Vec::new();
for _ in 0..25 {
let input = input.clone();
let instance_with_store = TestWasm::Test.instance();
let instance_with_store = TestWasm::Test.unmetered_instance();
let instance = instance_with_store.instance.clone();
let store = instance_with_store.store.clone();
let jh = std::thread::spawn(move || {
Expand Down
6 changes: 6 additions & 0 deletions test/src/import.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
use crate::debug;
use crate::decrease_points;
use crate::err;
use crate::pages;
use crate::short_circuit;
Expand Down Expand Up @@ -34,6 +35,11 @@ pub fn imports(store: &mut StoreMut, function_env: &FunctionEnv<Env>) -> Imports
function_env,
debug
),
"__hc__decrease_points_1" => Function::new_typed_with_env(
store,
function_env,
decrease_points
),
"__hc__guest_err_1" => Function::new_typed_with_env(
store,
function_env,
Expand Down
71 changes: 68 additions & 3 deletions test/src/test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ pub mod wasms;
use holochain_wasmer_host::prelude::*;
use test_common::SomeStruct;
use wasmer::FunctionEnvMut;
use wasmer_middlewares::metering::MeteringPoints;

pub fn short_circuit(
_env: FunctionEnvMut<Env>,
Expand Down Expand Up @@ -49,6 +50,31 @@ pub fn debug(
Ok(())
}

pub fn decrease_points(
mut function_env: FunctionEnvMut<Env>,
guest_ptr: GuestPtr,
len: Len,
) -> Result<u64, wasmer::RuntimeError> {
let (env, mut store_mut) = function_env.data_and_store_mut();
let points: u64 = env.consume_bytes_from_guest(&mut store_mut, guest_ptr, len)?;
let points_before = env.get_remaining_points(&mut store_mut)?;
let remaining_points = env.decrease_points(&mut store_mut, points)?;
let points_after = env.get_remaining_points(&mut store_mut)?;
assert_eq!(points_after, remaining_points);
env.move_data_to_guest(
&mut store_mut,
Ok::<(u64, u64), WasmError>(match (points_before, remaining_points) {
(
MeteringPoints::Remaining(points_before),
MeteringPoints::Remaining(remaining_points),
) => (points_before, remaining_points),
// This will error on the guest because it will require at least 1 point
// to deserialize this value.
_ => (0, 0),
}),
)
}

pub fn err(_: FunctionEnvMut<Env>) -> Result<(), wasmer::RuntimeError> {
Err(wasm_error!(WasmErrorInner::Guest("oh no!".into())).into())
}
Expand Down Expand Up @@ -91,7 +117,8 @@ pub mod tests {
vec![
"__hc__short_circuit_5".to_string(),
"__hc__test_process_string_2".to_string(),
"__hc__test_process_struct_2".to_string()
"__hc__test_process_struct_2".to_string(),
"__hc__decrease_points_1".to_string(),
],
module
.imports()
Expand Down Expand Up @@ -289,7 +316,7 @@ pub mod tests {
Err(runtime_error) => assert_eq!(
WasmError {
file: "src/wasm.rs".into(),
line: 100,
line: 101,
error: WasmErrorInner::Guest("oh no!".into()),
},
runtime_error.downcast().unwrap(),
Expand Down Expand Up @@ -330,7 +357,7 @@ pub mod tests {
assert_eq!(
WasmError {
file: "src/wasm.rs".into(),
line: 128,
line: 129,
error: WasmErrorInner::Guest("it fails!: ()".into()),
},
runtime_error.downcast().unwrap(),
Expand All @@ -339,4 +366,42 @@ pub mod tests {
Ok(_) => unreachable!(),
};
}

#[test]
fn decrease_points_test() {
let InstanceWithStore { store, instance } = TestWasm::Test.instance();
let dec_by = 1_000_000_u64;
let points_before: u64 = instance
.exports
.get_global("wasmer_metering_remaining_points")
.unwrap()
.get(&mut store.lock().as_store_mut())
.unwrap_i64()
.try_into()
.unwrap();

let (before_decrease, after_decrease): (u64, u64) = guest::call(
&mut store.lock().as_store_mut(),
instance.clone(),
"decrease_points",
dec_by,
)
.unwrap();

let points_after: u64 = instance
.exports
.get_global("wasmer_metering_remaining_points")
.unwrap()
.get(&mut store.lock().as_store_mut())
.unwrap_i64()
.try_into()
.unwrap();

assert!(before_decrease - after_decrease == dec_by);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor, optional:

assert_eq! here?

Then separate assertions on the next line so it's clear what failed if something goes wrong?

assert!(
points_before > before_decrease
&& before_decrease > after_decrease
&& after_decrease > points_after
);
}
}
Loading
Loading