wasm-compose: make im-rc an optional feature-guarded dependency

[soldair]

Hey folks. Long time lurker first time contributor.

Because of potential unsafety in im-rc's deps I've tried to make it optional for wasm-compose as it's impacting the import of wasmtime.

I imagine we were using im-rc for actual performance reasons. Is there a common pattern for this use case people do instead? Is swapping it for the default HashMap going to break anything other than it being slower?

I'd love any suggestions here as to how to contribute here. It's causing quite a bit of work explaining to unsafe reviewers at google why i might need an older bitmaps and sized-chunks.

[alexcrichton]

Thanks for the PR! Agreed trimming deps is a worthwhile endeavor, and agreed it makes sense in this case too. I've got a few thoughts on this as well:

1. For merging this PR, I think it'd be reasonable to drop the im-rc dependency entirely. The original rationale was indeed performance but in practice I don't think the perf corner cases have ever really showed up and additionally with wasm-compose being dis-favored relative to wac I think it's reasonable to having wasm-compose in a mode of "shouldn't add maintenance burden", where pulling in extra dependencies is maintenance burden. So for that, if you're willing, could you remove the dependency entirely (not even optional)? I'd be happy to approve/merge after that.
2. At a higher level the wasm-compose dependency in Wasmtime is only needed for the compile-time-builtins feature. That's additionally only needed for some of the functionality of compile-time-builtins, so we could add more feature gates in the wasmtime crate itself which would possibly remove this dependency entirely if your embedding/vendoring doesn't actually need the features.
3. At an even higher level, I'd be happy to help out if y'all need assistance or have questions or anything. Our intention is that Wasmtime's set of dependencies is proportional to the set of functionality enabled in Wasmtime, and for user experience we enable a lot of features by default but they're not all critical for functionality. I'd be happy to help out lending thoughts to which features would make sense to leave on vs off. Or if y'all have general Wasmtime/Rust questions I'd be happy to help too!

[pchickey]

(Edit: I didn't see Alex's comment before sending this, but it looks like we agree!)

I would rather drop the use of im-rc completely and use std's HashMap in all cases than use a feature here, unless someone with more context than me can help me understand why its needed.

The use of im_rc::HashMap was introduced by @dicej in #1261 . In the commit he described:

I've brought in im-rc as a dependency since it makes rolling back tentative
changes trivial. There may be more efficient approaches if that's a concern.

but I don't actually know what he means by rolling back tentative changes, and in what way im-rc beats out std there. The docs of im-rc seem to claim that operations are close enough to O(1) for most purposes, which std's HashMap doesn't guarantee.

The other thing worth saying about wasm-compose is that, while it isnt a completely dead project - I suspect there are still users of it somewhere, but its not a focus of development or use anymore. Its generally understood that wac is the long-term successor project, but development effort on component composition has more or less been on pause waiting for wasip3 to stabilize. The last time any nontrivial maintenance was done on wasm-compose was in #2169, and anything more substantial is at least a year prior to that - all other changes are just to keep it up with changes to the rest of the family, and no new feature development has been undertaken for proper implementations of the async component model stuff that I can see.

Given that wasm-compose is alone in taking this odd dep and wasm-compose isnt getting much use or maintenance anymore, and I expect that efforts will proceed on wac instead of wasm-compose in the future, my inclination is to make the simplest change possible that will simplify the dep tree here to just use std's HashMap.

[dicej]

I've brought in im-rc as a dependency since it makes rolling back tentative
changes trivial. There may be more efficient approaches if that's a concern.

but I don't actually know what he means by rolling back tentative changes, and in what way im-rc beats out std there.

The context here is matching resource imports to resource exports an an arbitrary component graph, which I did by speculatively matching them up and then backtracking if the match-up created a contradiction. Using an immutable data structure made that easier than manually undoing changes to a mutable map, and was cheaper than wholesale copying a map for each change.

That said, I have no objection to removing the dependency.

[soldair]

Thanks so much folks!
I pushed again. With just the removal of the dep.

@alexcrichton I really appreciate that thoughtful response we can definitely improve the feature selection internally.
I kept our features broad as well because i wanted new users to find things just worked and to test the limits of our rust import tooling 😅
I'll reach out for advice and to sync up.

I'll probably have a few more of these prs here and there. Excited to contribute more even.
I appreciate y'all's help!

[alexcrichton]

Sounds good!

And yeah if the cases that @dicej planned for show up I think we can brainstorm possible ideas at that point, but my hope is that by that point we'll surely have transitioned to wac... Time will tell!

[alexcrichton]

Also, follow-up question @soldair -- we don't have a timeline/cadence for releases here in this repository, is this something which would be helpful to get into Wasmtime ASAP? Or is a more gradual cadence ok?

[soldair]

Gradual is ok. I'm floating patches where needed.
The version skew between common deps of this and wasmtime is a bit of work. It's not all on wasmtools though.
I think across the rust ecosystem we are looking at few versions of wasmparser/encoder etc for our one version rule 😆 . I've been pondering how to improve this.

[pchickey]

wasm-tools versions churn pretty frequently because we release everything in lockstep as the aspects of wasm which are not yet standards ratified (mostly the component-model-async work lately, also the cooperative threads work building on that) are changing rapidly. For wasmparser and wasm-encoder I my rough guess is that, if you're not using those features (i.e. you don't inspect/construct those leaves of any of the relevant enums), you could be 10 or 20 or 30 releases back and all functionality is identical. Standardized wasm doesnt change, and wasmparser and wasm-encoder's API that could change separately from wasm proposals is awfully stable.

If you opt out of needing the async component model and coop threads in wasmtime, it might be possible for a wit-bindgen and wasmtime feature to enable a dep on an "LTS" version of wasm-tools that changes much less frequently. Once those features stabilize in the standards arena, their wasm-tools representations will stabilize as well, and we can then make a new LTS that provides them.

Coming up with an LTS release process and threading it through those repos under a flag would be a significant engineering project, and I don't think I could volunteer any time beyond reviewing given my other priorities right now, but it could create a situation where we could help you (and the rest of the community) end up with less churn there if its desirable. It would hopefully be about the same amount of work of you creating a private patch set to achieve the same internally, and have the benefit of Alex getting to review it.