Implement resource borrow and transfer trampolines#10
Conversation
|
Thanks for your work! This PR looks to be on the right track so far. |
juntyr
force-pushed
the
resource-transfer
branch
from
7331059 to
cf1430e
Compare
|
Sorry for the delay - as the stubs were enough to get my use-case running it took a while to find some time to get back to this PR and implement it properly. Now that bytecodealliance/jco#371 has landed I have a good reference for the actual implementation :) |
| let from_table = &mut table_array[from_rid]; | ||
| let handle_borrow = from_table.get(*handle)?; | ||
| let handle_rep = handle_borrow.rep; | ||
| // FIXME: wrong condition, should be if from_rid is imported |
There was a problem hiding this comment.
@DouglasDwyer I am unsure about how to implement this part (and about whether I understand Jco's code here https://github.com/bytecodealliance/jco/blob/1420aad1679418da6037bb787a9130b88453bcd3/crates/js-component-bindgen/src/intrinsics.rs#L198-L211). Jco checks whether the resource is imported or local to this instance - is this information already stored somewhere / how could I get it?
There was a problem hiding this comment.
Hey @juntyr, thanks again for your hard work on this! I do not believe that I store whether the resource is imported or local to an instance anywhere currently.
I would need to do a bit more reading to understand why the handle is not cleared if the resource is local to the instance. Why would the behavior be different?
I am wondering if this check has to do with the "simplification" where local borrows aren't tracked? That simplification isn't implemented at all in this crate right now.
There was a problem hiding this comment.
I hadn’t considered that “simplification” before but I think you’re right that this might be the reason for this if statement in Jco. Without it, would a borrow transfer just decrease the lend count in the “from” resource and create a new resource instance for “to”? Is this the purpose of the lend count? If we always create a new resource instance, we also loose the second if statement which just uses the inner representation inside the defining instance, otherwise we might gain layers of indirection which could mess with that instance‘s assumption that the representation is just a pointer?
juntyr
left a comment
juntyr
left a comment
There was a problem hiding this comment.
I've made some more progress in filling out the trampolines but there are still some unresolved questions and cleanup needed
…ll, and ResourceExitCall
juntyr
force-pushed
the
resource-transfer
branch
from
6667c30 to
7117bb0
Compare
juntyr
changed the title
|
@DouglasDwyer I've upgraded dependencies and cleaned up the code a bit. This PR works for my usecase at least, but I'd appreciate a review to see if I missed anything obvious. |
I think I triggered that bug once before as well but also just worked around it at the time. Do you still have a reproducible example you could share? Perhaps I can find the fix. |
I can maybe find a commit where it was happening in my project but with how its set up its probably easier to create a separate example from scratch. I'm not home now to do so though. |
|
@juntyr, I skimmed through the code and everything looks good. Unfortunately, my current focus is on other projects, and I don't have time to vet
Once those things are finished, I will happily merge this PR :) |
|
3 participants
This PR implements resource borrows and transfers for the wasm_component_layer. In particular, it:
This PR is only a first step, but - as can be seen from the implementation duration - has survived being used in my project for several months. Merging this PR will expose the added functionality to more users who can implement further usecases (e.g. other transcoder trampolines) as required.