-
Notifications
You must be signed in to change notification settings - Fork 824
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
Polymorphic host functions based on dynamic trampoline generation. #1217
Conversation
bors try |
tryBuild succeeded
|
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.
As discussed in private, I'll incorporate some code form #1018 to make it fit in a more dynamic environment.
…ions. This patch adds a new field in `Func`: `signature`. It contains the signature of the host function. For non-polymorphic host functions, the signature is computed from the `Args` and `Rets` implementation parameters at compile-time. For polymorphic host functions though, to be fully dynamic, the signature given to `new_polymorphic` is used in `Func` as the correct signature.
Please, check if #1225 can be included in this PR 🙂. |
feat(runtime-core) Allow dynamic signature for polymorphic host functions
bors try |
tryBuild succeeded |
bors try |
Co-Authored-By: Ivan Enderlin <[email protected]>
Co-Authored-By: Ivan Enderlin <[email protected]>
…ture/polymorphic-v2
bors try |
tryBuild succeeded |
let mut alloc = self.alloc.lock().unwrap(); | ||
let mut found = false; | ||
|
||
// Then, try invalidating that assumption... |
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.
How common is freeing blocks? And inserting new ones?
Should we instead store a single allocated region, plus a list of "free" regions, keeping consecutive free regions coalesced? This loop would then try to allocate out of the free list. We could even keep the free list sorted by size instead of start location, so that we can quickly find the smallest block that will fit?
Should we even bother reusing freed memory here at all? If I understand right, we produce at most one trampoline per imported function, which is a finite number for a given wasm instance? We could have one of these per instance or module, then just allocate off the end only and delete the whole thing when the instance is deleted?
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 free region method would be the ideal way to track dynamic allocation, as in common memory allocators. Here I just kept track of used blocks for simplicity, maybe it's good to change this.
The lifetime of DynamicFunc
is unbounded and we cannot associate it to an instance safely at creation time. Or do you think the public API should be refactored and DynamicFunc::new
should be made a method on Instance
?
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.
I was just thinking it'd be faster to run until the end before trying to use up spaces in the middle. The normal case is that this search will return zero free spaces and we don't need to do a scan until we're actually out of memory.
Talked about DynamicFunc::new
with Syrus and Mark, we agreed that putting them on Instance
is the wrong way to go. Functions can exist outside of instances anyways, and we're currently failing a spectest over that.
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.
I just implemented it in a simple way so that we would not increase our code's size too much with "yet another allocator". For performance optimization, can we port over an external allocator like wee_alloc
?
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.
Merging this first and optimizations on the executable memory allocator can be made in another PR.
let mut alloc = self.alloc.lock().unwrap(); | ||
let mut found = false; | ||
|
||
// Then, try invalidating that assumption... |
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.
I was just thinking it'd be faster to run until the end before trying to use up spaces in the middle. The normal case is that this search will return zero free spaces and we don't need to do a scan until we're actually out of memory.
Talked about DynamicFunc::new
with Syrus and Mark, we agreed that putting them on Instance
is the wrong way to go. Functions can exist outside of instances anyways, and we're currently failing a spectest over that.
bors r+ |
Build succeeded |
thanks @losfair for this feature. This was the missing piece for me to implement support for imported functions in my elixir-wrapper. I was about to ask you for this feature, when I spotted it in the changelogs. thanks ❤️ |
This PR implements polymorphic host functions by dynamically generating the "glue" code that translates platform arguments to an array in
runtime-core
.TODO:
Multiple return values.Deferring to a future multivalue PR.