-
Notifications
You must be signed in to change notification settings - Fork 14
Implement memory usage logging #17
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
Merged
Merged
Changes from 7 commits
Commits
Show all changes
12 commits
Select commit
Hold shift + click to select a range
36cfae2
feat: memory size calculation
fc51185
chore: pulled upstream
e34b4b7
chore: merge with upstream
57bfe19
test: added memory tests
047e55a
test: a wasm bin that should overflow the stack
56b443a
style: output stats order
1232fa9
Merge remote-tracking branch 'origin' into feat-memory-limit
1c9ee3a
fix: adapt to new terminology
hainsdominic 90f22e2
fix: adapt to new terminology
hainsdominic daa98ef
fix: adapt to new terminology
1c273ea
test: constants and positive-outcome test
bb21919
fix: sum up all memories
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,11 @@ | ||
| { | ||
| "input": { | ||
| "context": { | ||
| "suffix": "From Core!" | ||
| } | ||
| }, | ||
| "configuration": { | ||
| "message": "From Core!" | ||
| }, | ||
| "extension_point": "hello_world" | ||
hainsdominic marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| } | ||
Binary file not shown.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,11 @@ | ||
| { | ||
| "input": { | ||
| "context": { | ||
| "suffix": "From Core!" | ||
| } | ||
| }, | ||
| "configuration": { | ||
| "message": "From Core!" | ||
| }, | ||
| "extension_point": "hello_world" | ||
hainsdominic marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| } | ||
Binary file not shown.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a standard? Do we need to document this somewhere?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be clear, I'm talking about the
memorystring, not the method of measuring the associated memoryI don't see any use of the magic
memoryname in those docs 🤔 In fact the first link usesmemin some of their examplesThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it from here: https://github.com/bytecodealliance/wasmtime/blob/bffce37050abb22e3f07583a9a695ac790236f91/examples/memory.rs#L21
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems unlikely the function developer will have seen that before using this tool. We'll either need to
memoryname somewhereUh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.