Skip to content
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

wac_types::Package not having its own wac_types::Types requires re-parsing packages on every composition #85

Open
juntyr opened this issue Apr 16, 2024 · 7 comments

Comments

@juntyr
Copy link

juntyr commented Apr 16, 2024

In my usecase, I need to perform several compositions that each have a different "main" component but several util components that are the same (they implement WASI, similar to what wasi-virt does but even more constrained). Previously, I was only parsing these util packages once (using Package::from_bytes, storing them in a std::sync::OnceLock, and then using them in many compositions.

I just updated my code to the latest version of wac on git. Now that the wac_types::Types are inside the wac_graph::CompositionGraph, I can no longer cache (or at least I don't know how to) the util packages but need to re-parse them on every composition.

Perhaps there could be an extra utility function, e.g. wac_graph::CompositionGraph::register_package_with_types, to take in another wac_types::Types instance and to add all of these types to the composition graph's types and to remap the package's types into the composition graph's ones (I assume something like that was done before)?

Thank you for your help and all of your amazing work on these crates!

@peterhuene
Copy link
Member

Would a clear_nodes method on CompositionGraph that keeps the registered packages (and thus nothing to reparse), but deletes all nodes and edges help with your usage pattern?

@juntyr
Copy link
Author

juntyr commented Apr 16, 2024

Mmmm good question. There is always one different package, so just clearing one composition graph wouldn't help. But perhaps I could pre-generate a composition graph with all the utils packages pre-registered, store that in a OnceLock, and then clone it for every new composition (and the new package would just be added on top of that). I'll try that out and report back :)

@peterhuene
Copy link
Member

peterhuene commented Apr 16, 2024

While that might work just fine, it does seem like a less than ideal API experience.

Let me think more on this and see if I can come up with a solution where packages can be shared well between composition graphs, eliminating the need for reparses and, ideally, a lot of remapping as it can be a bunch of unnecessary allocations.

@peterhuene
Copy link
Member

I won't hold up a 0.1.0 release for this, but I think we can follow-up quickly with a 0.2.0 once we get the API around this right.

@juntyr
Copy link
Author

juntyr commented Apr 16, 2024

While that might work just fine, it does seem like a less than ideal API experience.

The approach does work, also on v0.1.0 :)

I won't hold up a 0.1.0 release for this, but I think we can follow-up quickly with a 0.2.0 once we get the API around this right.

Congratulations for publishing the first release!

@peterhuene
Copy link
Member

So here's what I'm currently thinking for addressing this:

  • Add a Types collection back to Package; remove the types parameter from Package::from_file and Package::from_bytes.
  • Make CompositionGraph::register_package take Into<Arc<Package>> for the package, which you can pass an Arc<Package> if you want to share packages between graphs (note: Package should already be Send and Sync).
  • The graph will keep track of whether or not its seen the package before (i.e. a package that was previously registered, deregistered, then registered again). If it hasn't seen the package before, it remaps the package's types into its own type collection.
  • define_type and import methods on CompositionGraph will translate, as necessary, a type coming from a currently registered package to its own remapped copy; thus, from users of the graph's point of view, there's still a singular Types collection (the graph's).

@peterhuene
Copy link
Member

I'm also toying with ways we can prevent remapping entirely.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants