-
Notifications
You must be signed in to change notification settings - Fork 344
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
Migrate to loupe
for determining the size of modules in memory
#959
Comments
Here is the line that panics because of the overflow: https://github.com/wasmerio/wasmer/blob/2.0.0-rc2/lib/types/src/entity/boxed_slice.rs#L159. Now we need to get a stack trace to understand how this is called. |
@Hywan I boiled it down to a MWE and it seems reproducible with just built-in use wasmer::{Cranelift, Module, Store, Universal};
static CONTRACT: &[u8] = include_bytes!("../hackatom.wasm");
#[test]
fn loupe_overflow_tests() {
let engine = Universal::new(Cranelift::default()).engine();
let store = Store::new(&engine);
let foo = Module::new(&store, &CONTRACT).unwrap();
loupe::size_of_val(&foo); // DON'T STAY CALM, DO PANIC!
} [dependencies]
loupe = "0.1.2"
wasmer = { git = "https://github.com/wasmerio/wasmer", tag = "2.0.0-rc2" } The Wasm file can be found here: I don't think we're going to investigate further right now since it's not a critical issue for us, but if we can help you along, let us know! |
OK I understand what's happening. Let me fix this :-). Thank you for your investigation! |
|
@Hywan could you create a ticket in Wasmer to track this and make visible to other users? This is not blocking us for now. Thanks! |
The 0.1.3 version of wasmerio/loupe is fixing your issue. I'm opening a PR on wasmerio/wasmer. |
Amazing, thanks! |
It includes this patch wasmerio/loupe#18 that fixes this bug CosmWasm/cosmwasm#959.
Revisiting this, just out of curiosity. Here is a comparison of our guesstimate of module sizes, versus
So, either we're underestimating module sizes, or loupe is over-estimating them. And who knows which is which and who is who. I think that, in any case, we should use loupe's value, as it's better to over-estimate the cache usage, than the opposite. |
Could you check the loupe values of |
Here are the numbers:
I think this makes sense. We're (probably) guesstimating the artifact size more or less accurately, and missing on the store size. I also noticed that |
One think you must be careful about when comparing sizes is that
pub fn size_of_val<T: MemoryUsage>(value: &T) -> usize {
<T as MemoryUsage>::size_of_val(value, &mut BTreeSet::new())
} Otherwise, you will need to use the same “memory tracker”, see the fn size_of_val(&self, tracker: &mut dyn MemoryUsageTracker) -> usize You can do that like this (not tested, almost pseudo-rust-ish-code): let mut tracker = BTree::new();
dbg!(module.store().size_of_val(&mut tracker));
dbg!(module.size_of_val(&mut tracker)); That way, you will re-use the same “memory tracker”, and you can more or less compare memory usages. I'm saying more or less because already visited pointers count for 1, so you are likely to notice small differences in your maths. |
Does My guesstimate only considers heap memory. That may very well be the reason for the divergence in the reported used memory. |
OK, I was playing with this these days, and I found a series of issues that may be causing the discrepancies:
All in all, this shows that |
So, I think we must clearly change the module size estimation method to |
Thank you for looking into this – sounds good! Is anything blocking us from wapping iut the implementation? Do you have the chance to open a PR for this, @maurolacy? |
OK, I will cut a PR with the changes. |
I confirm that we do take into account the mapped memory, https://github.com/wasmerio/wasmer/blob/7e914488383e2c0f3086b1b3aa60f277d60f5095/lib/vm/src/mmap.rs#L283-L289. |
Thanks for taking the time to check this. |
Currently, we use our own algorithm for guesstimating the size of WASM modules used in
packages/vm/src/cache.rs
. (Search formodule_size
andmodule.size
.)We should see if we can migrate to loupe instead.
The text was updated successfully, but these errors were encountered: