Reimplement how instance exports are stored/loaded#2984
Conversation
This commit internally refactors how instance exports are handled and fixes two issues. One issue is that when we instantiate an instance we no longer forcibly load all items from the instance immediately, deferring insertion of each item into the store data tables to happen later as necessary. The next issue is that repeated calls to `Caller::get_export` would continuously insert items into the store data tables. While working as intended this was undesirable because it would continuously push onto a vector that only got deallocated once the entire store was deallocate. Now it's routed to `Instance::get_export` which doesn't have this behavior. Closes bytecodealliance#2916 Closes bytecodealliance#2983
github-actions
Bot
added
the
wasmtime:api
Subscribe to Label Actioncc @peterhuene DetailsThis issue or pull request has been labeled: "wasmtime:api"Thus the following users have been cc'd because of the following labels:
To subscribe or unsubscribe from this label, edit the |
| exports, | ||
| types: Arc::clone(self.cur.module.types()), | ||
| signatures: Arc::clone(self.cur.module.signatures()), |
There was a problem hiding this comment.
Kind of unfortunate to add a new malloc call and a couple of atomic inc refs on the instantiation hot path. Did you happen to measure if this has any observable effect on instantiation times?
There was a problem hiding this comment.
This was actually already happening, although it was even worse. Previously we created an IndexMap with freshly allocated strings for all the names as well. This should perform fewer allocations (only one) as well as be a bit more optimized (it's an array, should be calloc-able).
The atomics I don't think really affect anything, they're on the order of a handful of ns when we shoot for a handful of us for instantiation.
|
I don't really feel familiar enough with this code to give an r+ but those two things jumped out at me when I looked at the PR. |
peterhuene
left a comment
peterhuene
left a comment
There was a problem hiding this comment.
This looks good to me!
I appreciate the plethora of comments in instantiate_raw too.
3 participants
This commit internally refactors how instance exports are handled and
fixes two issues. One issue is that when we instantiate an instance we
no longer forcibly load all items from the instance immediately,
deferring insertion of each item into the store data tables to happen
later as necessary. The next issue is that repeated calls to
Caller::get_exportwould continuously insert items into the store datatables. While working as intended this was undesirable because it would
continuously push onto a vector that only got deallocated once the
entire store was deallocate. Now it's routed to
Instance::get_exportwhich doesn't have this behavior.
Closes #2916
Closes #2983