Implement memory usage logging

[hainsdominic]

What are you trying to accomplish?

The run function can now calculate the memory usage of a Function and add it to the FunctionRunResult struct so it can display it. I also added a unit test to check for a stack overflow error using a benchmark Function that allocates 80MB on the stack. The limit of 256KB is imposed by wasmtime.

What should reviewers focus on?

I checked out @jianghong's branch about memory limit, but I logged it instead of interrupting the Function, because of this.

The impact of these changes

On Shopify

Easier to test. De-bloats the main.

On merchants

No change

On third-party apps

No change.

Tophat 🎩

You can test your own script by running cargo run --release -- <path/to/input.json> -s <path/to/script.wasm>.

Note that --release is mandatory to benchmark since the runtime is much shorter when the binary is optimized.

Additionally, I wrote automated tests (that are executed in an opt-3 environment), so cargo test will show the results of these tests.

Before you deploy

• I tophatted or tested this change.

[DuncanUszkay1]

Should we add the source code for those wasm files?

[DuncanUszkay1]

Is this a standard? Do we need to document this somewhere?

[hainsdominic]

Its the way they do it in the docs and in their book: https://docs.rs/wasmtime/0.37.0/wasmtime/struct.Memory.html
https://docs.wasmtime.dev/contributing-architecture.html?highlight=memory#linear-memory

[DuncanUszkay1]

To be clear, I'm talking about the memory string, not the method of measuring the associated memory

I don't see any use of the magic memory name in those docs 🤔 In fact the first link uses mem in some of their examples

[hainsdominic]

Got it from here: https://github.com/bytecodealliance/wasmtime/blob/bffce37050abb22e3f07583a9a695ac790236f91/examples/memory.rs#L21

[DuncanUszkay1]

It seems unlikely the function developer will have seen that before using this tool. We'll either need to

• (Ideal) Sum up all exported memory stores
• Document that they need to use the special memory name somewhere

[DuncanUszkay1]

Summing up everything they export seems like a reasonable solution, given that we have access to a list of memory export names. I'd rather not document a naming limitation if we could remove the limitation instead.

[saulecabrera]

This is not a Rust standard, is a standard across compiler infrastructures (LLVM, Binaryen). As Kevin mentioned, you could post-process your Wasm and change the name of the exported memory, not sure what you'd gain by doing so though. Once multiple memories are officially supported by the compiler toolchains I'm pretty sure we'd need to update this. For now this seems a good, standard approach.

[saulecabrera]

Also multiple memories is going to introduce some complexities to measure resource usage. I suspect that with multiple memories we'd need to implement the Resource Limiter because not all memories declared in a Wasm module are expected to be exported. So the current approach won't be accurate.

[DuncanUszkay1]

So the current approach won't be accurate.

True, but that's true regardless of whether or not we adopt the "memory" name standard since that's a name we're using to query exports. If that's an issue, let's open a separate separate thread about the approach of the PR in general.

On the topic of the "memory" name: In the latest commit Dominic added an approach that sums up all exported memories, which avoids this naming limitation. Is there a reason to re-add the limitation? It seems like a reasonable limitation if it's standard for LLVM and Binaryen, but it seems better to simply eliminate the limitation from my perspective.

[saulecabrera]

Yeah, the approach looks good. With the recent changes I don't see the need to reintroduce the "memory" naming standard. I emphasize on multi-memory because when we support it, for example for module linking, the approach here will need to differ considerably. Just want to make sure that this is on your radar.

[hainsdominic]

Should we add the source code for those wasm files?

we could, but I think it would bloat the repo

[DuncanUszkay1]

we could, but I think it would bloat the repo

How will we edit them without the source? 🤔 Is the idea that we'd just rewrite them if they ever needed to be updated? Are they simple enough that we can edit the WAT?

[hainsdominic]

For the test Functions, here is what they do/how I implemented them:

• hello_world, the simple hello world script
• hello_world_42_pages modified the requested number of pages at the end of the WAT file, so it wasn't obtained by compiling code
• sleeps: added a sleep for a duration of 42 seconds in the hello_world script
• stack_overflow: hello_world + allocated a huge array of u64 on the stack (80MB)

I don't think that any of them would be subject to change however

[jianghong]

Going to add @KevinRizzoTO for review since he worked on memory usage in our runtime engine. It would be good if this was aligned with how that is measured too

[KevinRizzoTO]

Going to add @KevinRizzoTO for review since he worked on memory usage in our runtime engine. It would be good if this was aligned with how that is measured too

Thanks for the add @jianghong. I actually like this implementation better than the one I ended up with. In short, I used the ResourceLimiter trait to hook into calls to the memory.grow instruction. I also used this as a means to get a rough estimation of memory used by saving the current value passed to the memory_growing method (see here). This generally works, but if the script never calls memory.grow then we get a reading of zero for memory usage. Not ideal, but my main goal was to get a rough estimate of what our limits should be so we can provide some sane defaults. In that regard, it works well enough for now until I can get the bandwidth to read the memory size directly from the export.

[DuncanUszkay1]

I actually like this implementation better than the one I ended up with

Are we planning to change the Runtime Engine implementation then? 🤔 Regardless of which implementation is better I think that we should have a plan towards consistency

[KevinRizzoTO]

@DuncanUszkay1 Not at this time. The service is going to be deprecated once we finish the work for inline execution so it's probably not worth it just yet. The tracking of total memory usage isn't user facing, just sent to statsd to graph in datadog

[DuncanUszkay1]

The tracking of total memory usage isn't user facing, just sent to statsd to graph in datadog

Gotcha. As long as we ensure that anything that is given to a developer is reproducible with the script runner I think we're good.

[DuncanUszkay1]

Great work on this 👏🏻