Cache wasmer instances and add cache stats #140
Conversation
| 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>>, |
There was a problem hiding this comment.
Ah... that does simplify things, as nothing is bound
lib/vm/src/cache.rs
Outdated
| @@ -48,6 +59,13 @@ where | |||
| modules, | |||
| wasm_path, | |||
| instances, | |||
| stats: Stats { | |||
There was a problem hiding this comment.
rust tip:
if you add #[derive(Default)] above the Stats definition, then you get this via Stats::Default()
There was a problem hiding this comment.
Thanks! Done in de489a9
| 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); |
There was a problem hiding this comment.
How about having Instance::recycle return {wasmer_instance, extern} and collapse the take_storage line above and simply the return value?
| 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); |
There was a problem hiding this comment.
Loving these stats. Not just for performance measurements, but to ensure the code is doing what we want.
| @@ -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> { | |||
There was a problem hiding this comment.
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.
|
|
||
| /// 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) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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!
webmaster128
changed the title
Closes #137
It turns out that reusing a wasmer instance from
Instanceis much easier than reusing the storage because the storage is needed to create the wasmer context, which makes things very complicated. This leaves the storage question untouched.