Refactor String usage in wasmtime_environ::Module

[fitzgen]

Do not hold regular Strings; instead use our own OOM-handling
wasmtime_core::alloc::String or, even better, an interned Atom from the
StringPool.

Also avoid IndexMap, as it doesn't handle OOMs.

Depends on

• Implement serde support for our OOM-handling SecondaryMap #12564
• Implement serde support for StringPool #12563
• Move StringPool to wasmtime-environ #12562

[alexcrichton]

Knee-jerk reaction before you have too many more PRs that depend on this -- IndexMap is a relatively fundamental dependency especially for components, so I'm not sure we'll be able to escape making an OOM friendly version. Or at least I recall my impression being that we have quite a lot of IndexMaps throughout compilation/etc. Is the usage pretty localized to just one or two locations though?

[fitzgen]

Knee-jerk reaction before you have too many more PRs that depend on this -- IndexMap is a relatively fundamental dependency especially for components, so I'm not sure we'll be able to escape making an OOM friendly version. Or at least I recall my impression being that we have quite a lot of IndexMaps throughout compilation/etc. Is the usage pretty localized to just one or two locations though?

IndexMap is either index_map::IndexMap or a BTreeMap depending on wasmparser configuration:

wasmtime/crates/environ/src/prelude.rs

Line 30 in 755979d

pub use wasmparser::collections::{IndexMap, IndexSet};

std/alloc does not provide a way to fallibly reserve/insert/allocate for BTreeMaps.

And index_map::IndexMap does not provide a try_reserve method either. Additionally, index_map::IndexMap's Deserialize implementation requires that the key be Clone, which our OOM-handling Strings are most definitely not.

So it seems like continuing to use IndexMap is going to be an uphill battle.

We could make our own IndexMap that is a full implementation, rather than simply a newtype wrapper over either of those existing implementations. But that also seems pretty annoying and like more of a maintenance burden than is ideal... But maybe not, given that what I did with exports here is basically inlining/hard-coding one specific instance of that?

Thoughts?

[alexcrichton]

One possible fix might be to use indexmap::IndexMap (directly from the crate, not wasmparser). That should support no_std and it looks like it's got a try_reserve to work for us as well. For Deserialize wouldn't we be writing custom impls as opposed to using any built-in ones too?

I guess what I'm saying is that my leaning is that we should have an indexmap::IndexMap wrapper the same way we have a Vec wrapper and then replace the usage of wasmparser::IndexMap with that fallible IndexMap.

[fitzgen]

I guess what I'm saying is that my leaning is that we should have an indexmap::IndexMap wrapper the same way we have a Vec wrapper and then replace the usage of wasmparser::IndexMap with that fallible IndexMap.

Okay, that has been done and I've pushed a new commit that switches this back to using IndexMap instead of multiple {Primary,Secondary}Maps.

Also this depends on these other PRs now:

• Implement serde support for our OOM-handling IndexMap #12580
• Remove unused wasmtime::runtime::vm::Instance::exports method #12579