bytecodealliance / wasm-tools

CLI and Rust libraries for low-level manipulation of WebAssembly modules
Apache License 2.0
1.35k stars 245 forks source link

wasm-compose: Elide imports from a component that are made up of only types #1195

Open thomastaylor312 opened 1 year ago

thomastaylor312 commented 1 year ago

While trying to implement https://github.com/wasmCloud/wasmCloud/issues/497, we discovered that shared types (defined in an interface) bubble up as imports to the top level when composing. For example, given an interface that looks like this:

package wasmcloud:messaging@0.1.0

interface types {
    record broker-message {
        subject: string,
        body: option<list<u8>>,
        reply-to: option<string>,
    }
}

interface handler {
    use types.{broker-message}
    handle-message: func(msg: broker-message) -> result<_, string>
}

interface consumer {
    use types.{broker-message}

    request: func(subject: string, body: option<list<u8>>, timeout-ms: u32) -> result<broker-message, string>
    request-multi: func(subject: string, body: option<list<u8>>, timeout-ms: u32, max-results: u32) -> result<list<broker-message>, string>
    publish: func(msg: broker-message) -> result<_, string>
}

You can have 2 interfaces that both depend on the shared types. When trying to virtualize (i.e. through virtual platform layering) a component that has a world like this:

world messaging {
    import consumer
    export handler
}

You end up with a bunch of dangling imports of the types interface. For example, this is the world after composing several of the virtualized interfaces:

package root:component

world root {
  import wasmcloud:messaging/types@0.1.0
  // ^^ This shouldn't be here since it is only types
  import wasi:io/streams
  import wasi:filesystem/types
  import wasi:filesystem/preopens
  import wasi:cli/environment
  import wasi:cli/exit
  import wasi:cli/stdin
  import wasi:cli/stdout
  import wasi:cli/stderr
  import wasi:cli/terminal-input
  import wasi:cli/terminal-output
  import wasi:cli/terminal-stdin
  import wasi:cli/terminal-stdout
  import wasi:cli/terminal-stderr
  import wasmcloud:bus/lattice
  import wasmcloud:bus/host

  export wasmcloud:bus/guest
}

After talking to @peterhuene, he said that we could check if all components being composed have the same import, and if there are no functions in the interface, we can elide it. We would also need to generate type instantiations that could be passed in to satisfy those types

alexcrichton commented 1 year ago

This seems reasonable to me as well, but one gotcha will be that resource types still need to be imported, so it won't be quite as simple as ignoring all types.

peterhuene commented 1 year ago

After thinking about this a little bit more, I think the fix probably belongs in wit-component when we're creating a component from a core module, as I can't think of a reason why we would want these in the resulting component.

It would also transitively eliminate the issue from wasm-compose.

@alexcrichton for the elision, would a check of "if the imported interface has no functions and only non-resource types, elide encoding" be sufficient?

alexcrichton commented 1 year ago

Hm on second thought I'm not actually sure where this would best go. Currently in wit-component it already eliminates all dead types (or at least it should) and only "live" types used by something should remain. That can lead to type-only imports but that's required to reflect the WIT structure to be able to recover WIT and merge things later on with equivalent type definitions.

So in that sense I thought that maybe wasm-compose could elide the types if they're not actually used by any other imports, although this sounds like something wasm-compose probably already does so I'm less certain that this may be possible to do.

Otherwise though the check you mention @peterhuene should be sufficient yeah

peterhuene commented 1 year ago

wasm-compose doesn't elide unused types as it respects the atomic nature of components: an inner component needs an import of a types-only instance for instantiation, so, where otherwise unspecified, it'll hoist that import to the composed component to satisfy the instantiation argument internally.

peterhuene commented 1 year ago

To fix this in wasm-compose, we would need to detect any outer imports of these concrete-type-only instances, create a dummy instance internally from a bag-of-matching-type-exports, and then feed that instance through to any inner component that needs the originally-imported instance for instantiation; we need to do this because wasm-compose should not rewrite any of the inner components.

Saying that makes me think we're abusing imports for referencing foreign types here; these types are definitely not participating in instantiation, only serving as markers so that bindings can dedup type definitions, which is strictly a build-time concern.

I am now wondering if the component model needs a way to define a type with an identifier, e.g. (type (id "foo:bar/baz") (record ...)) so that bindings can dedup the type, but the resulting component won't have an import of that type because it isn't needed as an instantiation argument.

Relatedly I think we really need to come up with a way of having top-level types in WIT.

alexcrichton commented 1 year ago

Another thought to perhaps throw in the ring: Wasmtime for example does not require types to be specified at instantiation time, only resource types. I've talked with Luke in the past about codifying that in the component model itself. By altering the instantiation rules I think we could keep the type structure that we have today but compositions wouldn't have to specify type-only instances (unless they have resources, and even then they'd only specify the resource types, nothing else)

Saying that makes me think we're abusing imports for referencing foreign types here; these types are definitely not participating in instantiation

Question from me, is the types-only instance here actually used for anything? I'd expect the types to be aliased and used by other interface imports and their functions, but if that's not the case then they're still required to match the structure of the original WIT, but if not then they're definitely unnecessary.

peterhuene commented 1 year ago

By altering the instantiation rules I think we could keep the type structure that we have today but compositions wouldn't have to specify type-only instances...

While I agree that would fix the problem, I worry that the validation rules for this would be complex to the point of a code smell (i.e. ignore concrete type imports, but also "only concrete type exporting" instances, in instantiations) just to avoid having to represent an "identifiable" concrete type in a different manner.

Additionally, speaking from a high-level "components as functions" viewpoint, it would also be a bit strange to have these "parameters" (i.e. imports) that are entirely ignored at runtime.

Personally, I would prefer to have imports always be used in instantiation (i.e. restrict validation of type imports to(sub resource) bounds or (eq ty) bounds where ty is (transitively) (sub resource)) and use something other than import to give an "identifier" to a concrete, non-resource type. But then again, I haven't fully thought through that in a Luke-like manner.

Question from me, is the types-only instance here actually used for anything?

I would assume it was prior to composition: one of the components being composed had an import of the types-only interface and an import of one of the implementation interfaces that reference those types, but wasm-compose was used to satisfy the import of the implementation interface via an instance of a different component that exports the implementation interface, leaving only the types-only interface being imported. Hence, it is being hoisted to a top-level import to satisfy the instantiation.

alexcrichton commented 1 year ago

Oh the validation here is actually pretty simple; it boils down to basically ignoring eq-bounded type imports with some extra fluff around if you do actually provide it then it has to match. I do agree though that it's a bit odd that you "take arguments" but then don't actually require them to be passed.

I'm also curious to dig in more though to your thinking about id-based types and top-level types. Could you sketch what you're thinking in a sample *.wit and *.wat perhaps?

Hence, it is being hoisted to a top-level import to satisfy the instantiation.

Ah ok makes sense. In that case it sounds like this could be elided by wasm-compose, although as you pointed out given the current instantiation rules wasm-compose would still have to manufacture an instance-of-types somehow (via bag-of-exports or also via component instantiation like wit-component does)

As a second question though, how come this ended up being a problem? I suspect that these type-only instances will get more rare in the future as most things transition to resources, so if this is a problem then that may need addressing regardless.

peterhuene commented 1 year ago

I'm also curious to dig in more though to your thinking about id-based types and top-level types. Could you sketch what you're thinking in a sample .wit and .wat perhaps?

I haven't thought deeply enough about it yet to really expand on it; let me see what I can come up with.

As a second question though, how come this ended up being a problem?

I'm unsure if it is an actual problem with running the composed component (@thomastaylor312 can elaborate) or if there was a general confusion over why the types-only import remains in the composed component despite having "erased" the conceptual dependency via virtualization.

I suspect, though, that there was an actual problem instantiating the component in a host that doesn't supply a superfluous wasmcloud:messaging/types@0.1.0 import when instantiating the component; while we ignore type imports in Wasmtime, do we ignore instances structured like this one to the point where the instance import itself is ignored?

alexcrichton commented 1 year ago

I think we should ignore instances, yeah. I thought we had tests for this but I can't seem to find them at this time. Testing locally though with small *.wast files it should be the case that empty instances and instances-of-types don't need to be passed for Wasmtime to instantiate at runtime.

thomastaylor312 commented 1 year ago

I'm unsure if it is an actual problem with running the composed component (@thomastaylor312 can elaborate) or if there was a general confusion over why the types-only import remains in the composed component despite having "erased" the conceptual dependency via virtualization.

It is a little of both here. It has been about 3-4 weeks since we tried to run it, but when we did, it couldn't instantiate it. I'll try again today and let you know. But the latter issue around confusion of having an import we essentially "erased" is definitely something we should document. If I can get the component instantiated and running today, then it definitely is a less-pressing issue than originally thought. I'll report back

thomastaylor312 commented 1 year ago

Ok, just tested it and it is working, so it does run now! 🎉. So at this point it is a cosmetic/understanding thing when those imports bubble up to the top level