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.

Sign up for GitHub

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

Jump to bottom

Cache wasmer instances and add cache stats #140

Merged
merged 6 commits into from
Jan 31, 2020
Merged

Cache wasmer instances and add cache stats #140

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
6aa28e1
Rename Storage type T -> S
webmaster128 Jan 31, 2020
983e7d6
Rename instance -> wasmer_instance
webmaster128 Jan 31, 2020
9c2b7b3
Store wasmer_runtime_core::Instance in cache
webmaster128 Jan 31, 2020
7ffaaef
Help understanding PhantomData in Instance
webmaster128 Jan 31, 2020
083c34b
Implement cache stats
webmaster128 Jan 31, 2020
de489a9
Use some useful trail implementations for Stats
webmaster128 Jan 31, 2020
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
64 changes: 57 additions & 7 deletions lib/vm/src/cache.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
use std::fs::create_dir_all;
use std::marker::PhantomData;
use std::path::PathBuf;

use lru::LruCache;
Expand All @@ -16,10 +17,20 @@ use crate::wasm_store::{load, save, wasm_hash};
static WASM_DIR: &str = "wasm";
static MODULES_DIR: &str = "modules";

struct Stats {
hits_instance: u32,
hits_module: u32,
misses: u32,
}

pub struct CosmCache<S: Storage + 'static, A: Api + 'static> {
wasm_path: PathBuf,
modules: FileSystemCache,
instances: Option<LruCache<WasmHash, Instance<S, A>>>,
instances: Option<LruCache<WasmHash, wasmer_runtime_core::Instance>>,
Copy link
Member

Choose a reason for hiding this comment

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

Ah... that does simplify things, as nothing is bound

stats: Stats,
// Those two don't store data but only fix type information
type_storage: PhantomData<S>,
type_api: PhantomData<A>,
}

impl<S, A> CosmCache<S, A>
Expand Down Expand Up @@ -48,6 +59,13 @@ where
modules,
wasm_path,
instances,
stats: Stats {
Copy link
Member

Choose a reason for hiding this comment

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

rust tip:

if you add #[derive(Default)] above the Stats definition, then you get this via Stats::Default()

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks! Done in de489a9

hits_instance: 0,
hits_module: 0,
misses: 0,
},
type_storage: PhantomData::<S> {},
type_api: PhantomData::<A> {},
})
}

Expand Down Expand Up @@ -78,30 +96,31 @@ where

// pop from lru cache if present
if let Some(cache) = &mut self.instances {
let val = cache.pop(&hash);
if let Some(inst) = val {
inst.leave_storage(Some(deps.storage));
return Ok(inst);
if let Some(cached_instance) = cache.pop(&hash) {
self.stats.hits_instance += 1;
return Ok(Instance::from_wasmer(cached_instance, deps));
}
}

// try from the module cache
let res = self.modules.load_with_backend(hash, backend());
if let Ok(module) = res {
self.stats.hits_module += 1;
return Instance::from_module(&module, deps);
}

// fall back to wasm cache (and re-compiling) - this is for backends that don't support serialization
let wasm = self.load_wasm(id)?;
self.stats.misses += 1;
Instance::from_code(&wasm, deps)
}

pub fn store_instance(&mut self, id: &[u8], instance: Instance<S, A>) -> Option<Extern<S, A>> {
if let Some(cache) = &mut self.instances {
let hash = WasmHash::generate(&id);
let storage = instance.take_storage();
let api = instance.api; // copy it
cache.put(hash, instance);
let (wasmer_instance, api) = Instance::recycle(instance);
Copy link
Member

Choose a reason for hiding this comment

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

How about having Instance::recycle return {wasmer_instance, extern} and collapse the take_storage line above and simply the return value?

cache.put(hash, wasmer_instance);
if let Some(storage) = storage {
return Some(Extern { storage, api });
}
Expand Down Expand Up @@ -148,6 +167,37 @@ mod test {
}
}

#[test]
fn finds_cached_module() {
let tmp_dir = TempDir::new().unwrap();
let mut cache = unsafe { CosmCache::new(tmp_dir.path(), 10).unwrap() };
let id = cache.save_wasm(CONTRACT_0_7).unwrap();
let deps = dependencies(20);
let _instance = cache.get_instance(&id, deps).unwrap();
assert_eq!(cache.stats.hits_instance, 0);
assert_eq!(cache.stats.hits_module, 1);
assert_eq!(cache.stats.misses, 0);
}

#[test]
fn finds_cached_instance() {
let tmp_dir = TempDir::new().unwrap();
let mut cache = unsafe { CosmCache::new(tmp_dir.path(), 10).unwrap() };
let id = cache.save_wasm(CONTRACT_0_7).unwrap();
let deps1 = dependencies(20);
let deps2 = dependencies(20);
let deps3 = dependencies(20);
let instance1 = cache.get_instance(&id, deps1).unwrap();
cache.store_instance(&id, instance1);
let instance2 = cache.get_instance(&id, deps2).unwrap();
cache.store_instance(&id, instance2);
let instance3 = cache.get_instance(&id, deps3).unwrap();
cache.store_instance(&id, instance3);
assert_eq!(cache.stats.hits_instance, 2);
Copy link
Member

Choose a reason for hiding this comment

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

Loving these stats. Not just for performance measurements, but to ensure the code is doing what we want.

assert_eq!(cache.stats.hits_module, 1);
assert_eq!(cache.stats.misses, 0);
}

#[test]
fn init_cached_contract() {
let tmp_dir = TempDir::new().unwrap();
Expand Down
30 changes: 15 additions & 15 deletions lib/vm/src/context.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,50 +74,50 @@ pub fn do_human_address<A: Api>(api: A, ctx: &mut Ctx, canonical_ptr: u32, human

/** context data **/

struct ContextData<T: Storage> {
data: Option<T>,
struct ContextData<S: Storage> {
Copy link
Member

Choose a reason for hiding this comment

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

Yeah, good rename. Generally in rust, all single generics are named T by convention. I found <T, U, V> a bit confusing, rather use S, A, etc in the code to make it clearer. Only did that when there were multi-args, but nice here for consistency.

data: Option<S>,
}

pub fn setup_context<T: Storage>() -> (*mut c_void, fn(*mut c_void)) {
pub fn setup_context<S: Storage>() -> (*mut c_void, fn(*mut c_void)) {
(
create_unmanaged_storage::<T>(),
destroy_unmanaged_storage::<T>,
create_unmanaged_storage::<S>(),
destroy_unmanaged_storage::<S>,
)
}

fn create_unmanaged_storage<T: Storage>() -> *mut c_void {
let data = ContextData::<T> { data: None };
fn create_unmanaged_storage<S: Storage>() -> *mut c_void {
let data = ContextData::<S> { data: None };
let state = Box::new(data);
Box::into_raw(state) as *mut c_void
}

unsafe fn get_data<T: Storage>(ptr: *mut c_void) -> Box<ContextData<T>> {
Box::from_raw(ptr as *mut ContextData<T>)
unsafe fn get_data<S: Storage>(ptr: *mut c_void) -> Box<ContextData<S>> {
Box::from_raw(ptr as *mut ContextData<S>)
}

fn destroy_unmanaged_storage<T: Storage>(ptr: *mut c_void) {
fn destroy_unmanaged_storage<S: Storage>(ptr: *mut c_void) {
if !ptr.is_null() {
// auto-dropped with scope
let _ = unsafe { get_data::<T>(ptr) };
let _ = unsafe { get_data::<S>(ptr) };
}
}

pub fn with_storage_from_context<T: Storage, F: FnMut(&mut T)>(ctx: &Ctx, mut func: F) {
let mut storage: Option<T> = take_storage(ctx);
pub fn with_storage_from_context<S: Storage, F: FnMut(&mut S)>(ctx: &Ctx, mut func: F) {
let mut storage: Option<S> = take_storage(ctx);
if let Some(data) = &mut storage {
func(data);
}
leave_storage(ctx, storage);
}

pub fn take_storage<T: Storage>(ctx: &Ctx) -> Option<T> {
pub fn take_storage<S: Storage>(ctx: &Ctx) -> Option<S> {
let mut b = unsafe { get_data(ctx.data) };
let res = b.data.take();
mem::forget(b); // we do this to avoid cleanup
res
}

pub fn leave_storage<T: Storage>(ctx: &Ctx, storage: Option<T>) {
pub fn leave_storage<S: Storage>(ctx: &Ctx, storage: Option<S>) {
let mut b = unsafe { get_data(ctx.data) };
// clean-up if needed
let _ = b.data.take();
Expand Down
41 changes: 26 additions & 15 deletions lib/vm/src/instance.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,9 +20,10 @@ use crate::errors::{ResolveErr, Result, RuntimeErr, WasmerErr};
use crate::memory::{read_region, write_region};

pub struct Instance<S: Storage + 'static, A: Api + 'static> {
instance: wasmer_runtime_core::instance::Instance,
wasmer_instance: wasmer_runtime_core::instance::Instance,
pub api: A,
storage: PhantomData<S>,
// This does not store data but only fixes type information
type_storage: PhantomData<S>,
}

impl<S, A> Instance<S, A>
Expand Down Expand Up @@ -70,46 +71,56 @@ where
}),
},
};
let instance = module.instantiate(&import_obj).context(WasmerErr {})?;
let wasmer_instance = module.instantiate(&import_obj).context(WasmerErr {})?;
Ok(Instance::from_wasmer(wasmer_instance, deps))
}

pub fn from_wasmer(wasmer_instance: wasmer_runtime_core::Instance, deps: Extern<S, A>) -> Self {
let res = Instance {
instance,
api,
storage: PhantomData::<S> {},
wasmer_instance: wasmer_instance,
api: deps.api,
type_storage: PhantomData::<S> {},
};
res.leave_storage(Some(deps.storage));
Ok(res)
res
}

/// Takes ownership of instance and decomposes it into its components.
/// The components we want to preserve are returned, the rest is dropped.
pub fn recycle(instance: Self) -> (wasmer_runtime_core::Instance, A) {
Copy link
Member

Choose a reason for hiding this comment

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

What about adding take_storage in here, which is tied to the wasmer_instance context.
So it fully decomposes all the info we put in.

Copy link
Member Author

Choose a reason for hiding this comment

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

I liked the idea. However, when thinking about it more I figured that this would make Instance construction and Instance::recycle asymmetric. This is because no representation of storage is owned by the instance. I think when we manually group storage and instance using leave_storage, we should also manually ungroup then later.

Copy link
Member

Choose a reason for hiding this comment

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

The usage I see (from the high level) is:

Instance::from_module(&module, deps) or Instance::from_code(&wasm, deps) to construct it. This takes deps: Extern<S, A>.

When freeing, or recycling the instance, I would assume the inverse of something like:

let {wasmer_instance, deps} = instance::recycle();

which should return a comprable deps as when we constructed it.

On another note, I wonder if we can make leave_storage and take_storage private to just the module. On the public side, we could create a new instance with one of the from_xxx() methods, recycle it, or do some queries via instance.with_storage(&|store| { ... }).

This can be another cleanup PR, but I think will simplify the external API and the safety guarantees. (I really like the recycle idea)

Copy link
Member Author

Choose a reason for hiding this comment

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

The usage I see (from the high level) is

oh yeah, right, this way we have symmetry.

On another note, I wonder if we can make leave_storage and take_storage private to just the module.

Jap, that works!

(instance.wasmer_instance, instance.api)
}

pub fn get_gas(&self) -> u64 {
get_gas(&self.instance)
get_gas(&self.wasmer_instance)
}

pub fn set_gas(&mut self, gas: u64) {
set_gas(&mut self.instance, gas)
set_gas(&mut self.wasmer_instance, gas)
}

pub fn with_storage<F: FnMut(&mut S)>(&self, func: F) {
with_storage_from_context(self.instance.context(), func)
with_storage_from_context(self.wasmer_instance.context(), func)
}

pub fn take_storage(&self) -> Option<S> {
take_storage(self.instance.context())
take_storage(self.wasmer_instance.context())
}

pub fn leave_storage(&self, storage: Option<S>) {
leave_storage(self.instance.context(), storage);
leave_storage(self.wasmer_instance.context(), storage);
}

pub fn memory(&self, ptr: u32) -> Vec<u8> {
read_region(self.instance.context(), ptr)
read_region(self.wasmer_instance.context(), ptr)
}

// allocate memory in the instance and copies the given data in
// returns the memory offset, to be later passed as an argument
pub fn allocate(&mut self, data: &[u8]) -> Result<u32> {
let alloc: Func<u32, u32> = self.func("allocate")?;
let ptr = alloc.call(data.len() as u32).context(RuntimeErr {})?;
write_region(self.instance.context(), ptr, data)?;
write_region(self.wasmer_instance.context(), ptr, data)?;
Ok(ptr)
}

Expand All @@ -127,7 +138,7 @@ where
Args: WasmTypeList,
Rets: WasmTypeList,
{
self.instance.func(name).context(ResolveErr {})
self.wasmer_instance.func(name).context(ResolveErr {})
}
}

Expand Down