Stream: git-wasmtime

Topic: wasmtime / issue #6637 handle interface functions correct...

view this post on Zulip Wasmtime GitHub notifications bot (Jun 23 2023 at 22:45):

github-actions[bot] commented on issue #6637:

Subscribe to Label Action

cc @peterhuene

<details>
This 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 <code>.github/subscribe-to-label.json</code> configuration file.

Learn more.
</details>

view this post on Zulip Wasmtime GitHub notifications bot (Jun 24 2023 at 01:08):

dicej commented on issue #6637:

Seems reasonable to me +1

I'd be slightly averse though to a custom Rc-based linked list, so I'd lean more towards something Vec like such as this (feel free to take it or leave it though)

Sounds reasonable. I went for the linked list because I assumed there could be more than one sibling LinkerInstance alive at a a given moment and thus couldn't share the same Vec. But now I see that's not possible since it would create aliasing &muts.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 24 2023 at 02:35):

dicej commented on issue #6637:

I like @jameysharp's proposal, but Alex's won because it came with a patch :)

view this post on Zulip Wasmtime GitHub notifications bot (Jun 26 2023 at 14:30):

alexcrichton commented on issue #6637:

Oh dear I apologize, I saw comments from Jamey and updates from Joel afterwards and I naively didn't actually read the comments and just assumed that the updates handled them! After reading Jamey's comments in more detail I would agree with his suggestion and I think that'd be an excellent refactoring to have. I poked at it for a moment but it was going to get somewhat nontrivial with the matching implementation where the list-of-instances needs to be passed around as context. Nothing insurmountable of course but it might be a bit of a nontrivial refactoring.

I do think though that we won't be able to go to the constant-time implementation of this though because given the current API we always need to walk the path to the current instance in the provided &Component to figure out the type.

@jameysharp how would you feel about deferring your suggestion to an issue? (and/or how do you feel about the current state of the PR as well)

view this post on Zulip Wasmtime GitHub notifications bot (Jul 05 2023 at 16:35):

jameysharp commented on issue #6637:

I'm happy to defer to your judgement, both of you. I think this isn't performance-critical, right? So making it correct is the only important thing right now, and my impression is that this PR does that as-is.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 05 2023 at 22:54):

alexcrichton commented on issue #6637:

Ok sounds good :+1:. This isn't performance critical yeah so I'll merge this for now and we can follow-up later as necessary

Last updated: Dec 23 2024 at 13:07 UTC