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
Polymorphic host functions based on dynamic trampoline generation. #1217
Polymorphic host functions based on dynamic trampoline generation. #1217
Changes from all commits
2fe6e6f
12373bb
5f4561e
7b0f7ee
ad20a00
2ee1e80
644755f
2020901
b67acbc
b7c9c18
80f824e
40d823e
96d9e39
a0ea1af
262d431
292e42a
a438a64
b0877b2
eb89720
72e6a85
32915f0
31a72e5
6516243
2ddf9ad
84179db
4012645
d443ad8
d9e744d
f499dea
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.
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 andDynamicFunc::new
should be made a method onInstance
?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 onInstance
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.