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.
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
experimental: configure custom memory allocator #2177
experimental: configure custom memory allocator #2177
Changes from all commits
bf30044
c9b4ff0
fc27033
609ec29
0283488
40af773
f2be6da
a4dd11a
2369008
a28156a
5dc8c55
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Food for thoughts, this method could potentially be removed in favor of
Grow(0)
.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.
Does this requirement make it impossible to implement a version of this interface backed by the Go heap?
It seems like the
sliceBuffer
implementation usingappend
would violate this constraint.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.
No:
max
to avoid relocations if you need such behavior.Note that my
mmap
implementation also reserves themax
address space, because it depends on memory never moving.PS: I can make this into a package (and, I guess, support Windows) if there's interest. It has interesting benefits. Makes startup/teardown slower, but can make growing memory faster, as it avoids copies. And with the trick of allocating 8GB, we could probably disable bounds checks altogether.
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.
The API as-is is quite easy to misuse, the interface implementation has no way of knowing if it's being called to allocated a shared memory segment or not, and the user may also not know if the underlying memory is shared-ready or not.
In my experience, this calls for separating the concepts, maybe we can even get compile-time checks to ensure that the right kind of memory is being used:
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.
Yes, point taken on “the allocator doesn't know if the memory is shared.”
I thought of adding a parameter, but then decided on just documenting. An implementor can and should specify if memories move when grown. Mine don't regardless of sharing.
The allocator is configured at module instantiation, so you should know by then if you need shared memory. Same as I know I need a memory that I can
mmap
but this is not compile time checked.Also with Go duck-typing your suggested API only differentiates the “kind” of memory if we add a dummy method.
Lastly, note that shared memories can still grow, they just can't move when grown. So the API is much the same.
With a large virtual address space it's perfectly feasible to reserve N×4GB of address space without committing any of it to memory, all on a 2GB “droplet" with no swap, and then organically decide at runtime which of the N gets 512MB while the other N-1 stay under 64MB.
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.
That's a bit more reliance on developers "getting it right" that I'm usually comfortable with; assumptions change over time especially in an evolving world like WebAssembly, I'd err on the side of caution and figure out how to leverage the compiler and API to be more future-proof. That being said, that's not a hill I'm willing to die on either, things working is better than things being perfect, I'm just trying to emphasize that most developers aren't Nuno and will likely get things wrong if the API allows for it ;)
I don't know if I'm suggesting differentiating via a dummy method, I'll have to think more about it, maybe the differences between linear and shared are more profound and could be expressed in the API.
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.
Future Nuno will thank you for it!
I'm all for expressing stuff with the type system, as long as it doesn't become clunky.
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.
Apologies because I'm a little late to the party, but since this is still in the
experimental
package it's maybe still OK to leave feedback; a few suggestions:MemoryBuffer
type sound more like an region or linear memory, we have an opportunity to improve the name to be less generic I think ("memory buffer" can describe about anything in a program), for example:etc...
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.
Thanks! I'm:
🤷 on the single method interface
👎 on the args struct (this is called in one place inside wazero, users implement it, don't call it)
👍 on renaming
MemoryBuffer
But I'll do whatever. I'm just ecstatic that I was able to
mmap
files and do WAL after one year. 🥳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.
Ah OK, your argument on the arg struct makes sense, if users won't be calling it directly the risk is low 👍
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.
We don't check ctx != nil elsewhere, so please simplify as
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.
Actually, there's a specific test case covering this:
wazero/internal/wasm/module_instance_test.go
Lines 87 to 89 in f31be30
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.
nil contexts should not be common in Go. There's even a lint to use context.Background() instead of passing nil because a lot of places assume a valid context. It may be reasonable to change this test.