Stream: git-wasmtime

Topic: wasmtime / PR #2897 Implement RFC 11: Redesigning Wasmtim...

Wasmtime GitHub notifications bot (May 11 2021 at 17:34):

alexcrichton opened PR #2897 from new-api to main:

This PR is an implementation of RFC 11 which is a redesign of Wasmtime's APIs.

Note: The RFC is not merged at this time, so this PR is purely meant for informational purposes and review. This should not merge until the RFC itself has been approved and merged.

I've gotten this impelmentation to the point now where it should be passing all in-tree tests. I think the performance is suitable for performance testing as well to ensure that it doesn't regress anything. I'd like to ideally get some review on this so by the time the RFC merges it's ready to go, and I'm also curious for design feedback which may influence the RFC itself as well.

I don't recommend paying attention to the commits themselves. They're all a mess and 95% of the changes are in the first commit. I plan on squashing everything and making a better commit message before merging.

Currently the only remaining TODO item is documentation. I have not yet update the API documentation of the wasmtime crate or the C API, and a lot of love needs to be poured into there. I would consider this a blocker before merging, I'm just working on the actual implementation itself first (Python/Go bindings are up next). I would also like to put more work into documenting some internal design decisions in inline comments, currently I don't think there's a sufficient amount. I plan on doing this with the general documentation pass I'll be doing later this week.

Wasmtime GitHub notifications bot (May 11 2021 at 17:35):

alexcrichton updated PR #2897 from new-api to main.

Wasmtime GitHub notifications bot (May 11 2021 at 17:35):

alexcrichton updated PR #2897 from new-api to main.

Wasmtime GitHub notifications bot (May 13 2021 at 05:02):

alexcrichton updated PR #2897 from new-api to main.

Wasmtime GitHub notifications bot (May 13 2021 at 05:13):

alexcrichton updated PR #2897 from new-api to main.

Wasmtime GitHub notifications bot (May 13 2021 at 05:15):

alexcrichton updated PR #2897 from new-api to main.

Wasmtime GitHub notifications bot (May 13 2021 at 14:12):

alexcrichton updated PR #2897 from new-api to main.

Wasmtime GitHub notifications bot (May 13 2021 at 15:40):

alexcrichton updated PR #2897 from new-api to main.

Wasmtime GitHub notifications bot (May 13 2021 at 15:42):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (May 13 2021 at 15:42):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (May 13 2021 at 15:42):

alexcrichton created PR review comment:

@fitzgen this is something I added here I mentioned on chat, basically the location that globals of type externref get their reference count decremented.

Wasmtime GitHub notifications bot (May 13 2021 at 15:42):

alexcrichton created PR review comment:

@peterhuene you might be interested in this. I inserted the call to drop_globals above to fix an issue where when an instance was dropped its externref globals weren't deallocated (this was a problem with the on-demand allocator as well).

In doing so, though, I realized the pooling allocator was attempting to double-drop them. This is fine because the "drop" zeros out the memory so next time around it looks like a null externref, but I figured it was best to fix that. Here I cleared the module/offsets field back to their original values when the instance was first initialized. I think that'll help prevent accidental leaks of data between instances or trying to interpret an instance as something it's not. Additionally this helps reclaim the memory used by Arc<Module> if this was the last reference to a module on the system.

Wasmtime GitHub notifications bot (May 13 2021 at 15:42):

alexcrichton created PR review comment:

This was something that I caught in the Static variant below and applied here. This previously prevented memories from growing to 65536 pages (exactly 4gb), but now this allows it. I've added a test that allocates this many pages to ensure this continues to work, but rereading the spec I believe this is allowed by the spec.

Wasmtime GitHub notifications bot (May 13 2021 at 15:42):

alexcrichton created PR review comment:

Technically the module can be inferred from InstanceHandle, but having a separate argument here as well made the implementations below much easier. The problem was that acquiring the module from InstanceHandle looked like we borrowed the entirety of InstanceHandle (since it was acquired through a method), which then prevents getting another mutable borrow on it.

For exaple during table initialization we'd want to call get_table which "borrows" the whole Instance. Having these be separate borrrows on input basically proves that the Arc<Module> is rooted elsewhere on the stack above this. That avoided having to clone() the module temporarily in all the below places.

Wasmtime GitHub notifications bot (May 13 2021 at 15:42):

alexcrichton created PR review comment:

@fitzgen I mentioned this on chat, but this was one of the important removals I made because technically globals should only work by reference so if you pull out an externref the reference count is increased.

Wasmtime GitHub notifications bot (May 13 2021 at 15:42):

alexcrichton created PR review comment:

@fitzgen this is probably worth pointing out here, but with &mut self we are now free from having to worry about recursive gc-in-gc so a lot of the bits and pieces here got simplified.

Wasmtime GitHub notifications bot (May 13 2021 at 15:42):

alexcrichton created PR review comment:

@fitzgen this is the second issue I mentioned on chat, this is the location where the reference count is now implemented relative to before when it was just copied over.

Wasmtime GitHub notifications bot (May 13 2021 at 15:42):

alexcrichton created PR review comment:

This was a major representation change for both memories and tables, the limiter is now passed in to all relevant operations rather than stored alongside each memory/table. This allows exclusive ownership to reside within the Store itself and, if necessary, it's passed down for operations. The limiter is also acquired through the new Store trait in the wasmtime_runtime crate

Wasmtime GitHub notifications bot (May 13 2021 at 15:42):

alexcrichton created PR review comment:

FWIW this was a semantic change where previously we'd allocate the memory, then realize we shouldn't have, and drop the memory if the limiter disallowed the new request. Now the limit happens before the allocation and we only proceed with alloating if the limiter allows it.

Wasmtime GitHub notifications bot (May 13 2021 at 15:42):

alexcrichton created PR review comment:

I've changed the representation of Static here to be more amenable to auto-impl of Send and Sync. I also folded the maximum field into the length of the base slice here.

My thinking is that this is faithful to the actual implementation as well where for the lifetime of this Memory it does indeed have exclusive access to the memory internally. There are other representations possible, too, which just require unsafe impl Send and such that can be implemented too.

Wasmtime GitHub notifications bot (May 13 2021 at 15:42):

alexcrichton created PR review comment:

FWIW before reading this file I'd recommend reading memory.rs above. Many of the changes here are similar to changes there, and I've commented the changes above to memories.

Wasmtime GitHub notifications bot (May 13 2021 at 15:42):

alexcrichton created PR review comment:

Note that the async bits here are a little different from before, but I think in a way that makes things a bit clearer. The async_cx is a "separately owned" structure which is connected (via unsafety) but not lifetimes to the original caller. This allows us to pass the entire caller to the future and let it close over it. The Wasmtime implementation, though, has to be careful that the fields referenced by async_cx may be aliased, hence their UnsafeCell wrappers.

Wasmtime GitHub notifications bot (May 13 2021 at 15:42):

alexcrichton created PR review comment:

This in particular was a very important removal from this crate. This removes the need for Any which removes the need for 'static from Store, allowing any T to be used with a Store.

The store pointer is learned via the VMContext and the *mut dyn wasmtime_runtime::Store contained within now.

Wasmtime GitHub notifications bot (May 13 2021 at 15:42):

alexcrichton created PR review comment:

I removed this in favor of TypedFunc::new_unchecked to make its presence a little less prominent.

Wasmtime GitHub notifications bot (May 13 2021 at 15:42):

alexcrichton created PR review comment:

I moved this here from Store, mostly just for aesthetic reasons, no real motivation other than that. This could arguably move to Config too, but it felt a little more appropriate here. Doesn't really matter one way or another though.

Wasmtime GitHub notifications bot (May 13 2021 at 15:42):

alexcrichton created PR review comment:

I moved this to tests/all/func.rs

Wasmtime GitHub notifications bot (May 13 2021 at 15:42):

alexcrichton created PR review comment:

This is an interesting bound. This is required by internals (notably the opaque_send() function). I think it would be possible to make this future conditionally Send if desired based on T (rather than always requiring T: Send), it'll just require some refactoring internally from what the current structure is. I'm not sure that it's all too useful to have that, so I went ahead and just added T: Send to the various async methods and if users really want non-Send futures with wasmtime we can come back and remove this via other means in the future.

Wasmtime GitHub notifications bot (May 13 2021 at 15:42):

alexcrichton created PR review comment:

Previously this function would have used with_last_info from the traphandlers module, but now it looks up the store from within the provided caller *mut VMContext. Note that previously the pointer could be null but I've now had it arranged such that this pointer is never null.

Wasmtime GitHub notifications bot (May 13 2021 at 15:42):

alexcrichton created PR review comment:

Note that this doesn't use AsContextMut so the liftetime can be explicitly closed over in the returned iterator

Wasmtime GitHub notifications bot (May 13 2021 at 15:42):

alexcrichton created PR review comment:

It's worth pointing out that I shifted functionality around here a bit. Now into_abi bakes in the compatible_with_store check as well as invoke takes/returns "ABI things" instead of raw values. This makes some more captures/closures Copy in the above _call function, but also notably means that the invoke function no longer needs a borrow on Store somewhow, which was much more difficult to provide.

Now the borrow is only required on into_abi here, then the call can happen without an explicit borrow on the store, and then re-ification back into actual values can happen after the call finishes.

Wasmtime GitHub notifications bot (May 13 2021 at 15:42):

alexcrichton created PR review comment:

Note that this doesn't take impl AsContext so we can tie to the precise lifetime.

Wasmtime GitHub notifications bot (May 13 2021 at 15:42):

alexcrichton created PR review comment:

This was a very satisfying thing to remove. This was originally added to prevent cycles, but now there are no cycles, so we could just delete everything related to this!

Wasmtime GitHub notifications bot (May 13 2021 at 15:42):

alexcrichton created PR review comment:

It's worth pointing out that I haven't implemented the sub_linker API from the RFC. I'm not sure if that's actually useful to implement or whether someone wants it. The intention was to preserve the functionality where today you can create global functions via Config and then each store-based Linker can add its own set of functions after-the-fact.

If that's used by applications and still desired I can add the sub_linker functionality here, but I figured it might be good to start with something simpler and only add that later if necessary.

Wasmtime GitHub notifications bot (May 13 2021 at 15:42):

alexcrichton created PR review comment:

Note that this requires Mut where it doesn't actually need it, but I left it here for the same reason as exports above.

Wasmtime GitHub notifications bot (May 13 2021 at 15:42):

alexcrichton created PR review comment:

R.e. Instance::exports above, this loop and this collect() are what I'd like to avoid in the future. This is an unnecessary allocation that instantiation performs which I think we can get rid of at no extra cost.

Wasmtime GitHub notifications bot (May 13 2021 at 15:42):

alexcrichton created PR review comment:

These are only used for HostFunc and correspond to the creation of a HostFunc and its deletion.

Wasmtime GitHub notifications bot (May 13 2021 at 15:42):

alexcrichton created PR review comment:

Note that I removed this in favor of a Store::limiter configuration API

Wasmtime GitHub notifications bot (May 13 2021 at 15:42):

alexcrichton created PR review comment:

The movement here was basically just movement of code from the ModuleRegistry to the GlobalModuleRegistry. These functions should only be accessed during trap creation so the slow locking should be ok, and it means that we didn't need the store during trap creation which was made a few things easier.

Data-representation-wise, though, nothing changed. The global registry already had all the information we needed.

Wasmtime GitHub notifications bot (May 13 2021 at 15:42):

alexcrichton created PR review comment:

A double-check on my logic here would be greatly appreciated.

Wasmtime GitHub notifications bot (May 13 2021 at 15:42):

alexcrichton created PR review comment:

This also, I think, keeps the usage of thread locals entirely contained within this file (except for manual management on async yields). One day I hope that we can actually remove the thread locals here entirely.

Wasmtime GitHub notifications bot (May 13 2021 at 15:42):

alexcrichton created PR review comment:

More eyes on this in particular would be appreciated.

Wasmtime GitHub notifications bot (May 13 2021 at 15:42):

alexcrichton created PR review comment:

It's also worth pointing out that Mut isn't technically needed here, yet. I realized that today, though, if you instantiate something we'll create a separate hash map in the wasmtime crate for all the exports (and their corresponding Extern value).

That feels inefficient and not necessary since the hash map is already stored within Arc<Module>. I'd like to improve this in the future so instantiation simply records the InstanceId and nothing else, and then lookup/fetching here actually mutates the store to create a new Func (or whatever it is) on-demand rather than doing it all up front.

This means that in the future mutability may be required, so I went ahead and added it here.

Wasmtime GitHub notifications bot (May 13 2021 at 15:42):

alexcrichton created PR review comment:

And another option of course is some sort of compile-time configuration where the number of bits allocated to the index is a build time variable which specific embeddings could configure. This is so obscure, though, that I'd prefer no one else have to ever worry about this.

Wasmtime GitHub notifications bot (May 13 2021 at 15:42):

alexcrichton created PR review comment:

This is, uh, very verbose. I'm not really sure how best to solve it though

Wasmtime GitHub notifications bot (May 13 2021 at 15:42):

alexcrichton created PR review comment:

(I think that GuestPtr needs to be rethought for other reasons too)

Wasmtime GitHub notifications bot (May 13 2021 at 15:42):

alexcrichton created PR review comment:

FWIW I think this is a real open question. I don't know what to do about this really. The way that GuestPtr works and how we need it Send and Sync necessitates this Mutex for now, but there's no threads here so there's no reason this should have a mutex. I'm hoping we can basically leave this to get improved later, probably by rethinking the way GuestPtr works.

Wasmtime GitHub notifications bot (May 13 2021 at 15:42):

alexcrichton created PR review comment:

After writing all this and thinking more about it, I think another possible solution could be:

• In Rust Stored<T> is 128 bits, 64 for the store and usize for the index within the store.
• In the C API we discard the 64 for the store and only have 64 for the usize, so the C API still uses 64-bit integers.

That would make the C API more unsafe than it is right now, but this change could be implemented backwards compatibly (even ABI-compatibly!). That makes me much less worried about the limits imposed here.

Wasmtime GitHub notifications bot (May 17 2021 at 23:50):

peterhuene requested peterhuene for a review on PR #2897.

Wasmtime GitHub notifications bot (May 18 2021 at 15:03):

alexcrichton requested pchickey for a review on PR #2897.

Wasmtime GitHub notifications bot (May 18 2021 at 15:27):

alexcrichton updated PR #2897 from new-api to main.

Wasmtime GitHub notifications bot (May 18 2021 at 15:36):

alexcrichton updated PR #2897 from new-api to main.

Wasmtime GitHub notifications bot (May 18 2021 at 17:16):

fitzgen created PR review comment:

// Data contained is always Send+Sync so these should be safe.

Wasmtime GitHub notifications bot (May 18 2021 at 17:16):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (May 18 2021 at 17:16):

fitzgen created PR review comment:

I kinda feel like we should remove this method at this point -- or alternatively give it an Ordering param -- because we use different orderings in different places, and its a bit subtle and something to be aware of, and so having this other method that makes the choice non-obvious and hidden from you seems a bit sketch.

Also, this method is only used twice now: once in an assertion and once in the strong_count method.

Wasmtime GitHub notifications bot (May 18 2021 at 17:16):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (May 18 2021 at 17:16):

fitzgen created PR review comment:

                    // Cast from `*mut T` to `*mut dyn Any` here.

That's a lot easier than the way I was doing the casting :-p

I swear casting to dyn is the thing I have the most trouble with in Rust at this point.

Wasmtime GitHub notifications bot (May 18 2021 at 17:16):

fitzgen created PR review comment:

Nice that we can use &mut self down here too, I hadn't realized that we could propagate these changes this deep.

Wasmtime GitHub notifications bot (May 18 2021 at 17:16):

fitzgen created PR review comment:

Very nice!!!

Wasmtime GitHub notifications bot (May 18 2021 at 17:16):

fitzgen created PR review comment:

Wait -- is this actually true? What about Drop impls that call back into Wasm? I guess they can't since they don't have a StoreContextMut. Nice.

Wasmtime GitHub notifications bot (May 18 2021 at 20:39):

alexcrichton updated PR #2897 from new-api to main.

Wasmtime GitHub notifications bot (May 18 2021 at 20:45):

alexcrichton updated PR #2897 from new-api to main.

Wasmtime GitHub notifications bot (May 18 2021 at 20:45):

alexcrichton created PR review comment:

Good point yeah, the wasmtime API of ExternRef::strong_count now mentions that it does a SeqCst load for synchronization purposes.

Wasmtime GitHub notifications bot (May 18 2021 at 20:45):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (May 18 2021 at 20:58):

alexcrichton updated PR #2897 from new-api to main.

Wasmtime GitHub notifications bot (May 18 2021 at 21:02):

pchickey submitted PR review.

Wasmtime GitHub notifications bot (May 18 2021 at 21:02):

pchickey created PR review comment:

I'm interested to hear more of your thoughts on this when you have some time!

Wasmtime GitHub notifications bot (May 18 2021 at 21:15):

alexcrichton updated PR #2897 from new-api to main.

Wasmtime GitHub notifications bot (May 19 2021 at 00:10):

pchickey submitted PR review.

Wasmtime GitHub notifications bot (May 19 2021 at 00:10):

pchickey created PR review comment:

can we additionally have an into_inner(self) -> T?

Wasmtime GitHub notifications bot (May 19 2021 at 14:42):

alexcrichton updated PR #2897 from new-api to main.

Wasmtime GitHub notifications bot (May 19 2021 at 14:43):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (May 19 2021 at 14:43):

alexcrichton created PR review comment:

Ah yeah I'd forgotten to come back and implement this, now implemented though!

Wasmtime GitHub notifications bot (May 19 2021 at 20:46):

alexcrichton updated PR #2897 from new-api to main.

Wasmtime GitHub notifications bot (May 19 2021 at 23:30):

peterhuene submitted PR review.

Wasmtime GitHub notifications bot (May 19 2021 at 23:30):

peterhuene created PR review comment:

Nit: should the multiple cfg inside of AsyncState be promoted to a single one on the type itself?

Wasmtime GitHub notifications bot (May 19 2021 at 23:30):

peterhuene created PR review comment:

I think this comment is now out of date given these changes.

Wasmtime GitHub notifications bot (May 19 2021 at 23:30):

peterhuene created PR review comment:

I'm confused by this call to drop; would you mind explaining its significance?

Wasmtime GitHub notifications bot (May 19 2021 at 23:30):

peterhuene created PR review comment:

Should we update this test or delete it rather than commenting it out?

Wasmtime GitHub notifications bot (May 19 2021 at 23:30):

peterhuene created PR review comment:

        // ever have to worry about the `Send`-ness of Wasmtime. If rustc says

Wasmtime GitHub notifications bot (May 19 2021 at 23:30):

peterhuene created PR review comment:

 * cannot be called concurrently. For example, functions which correspond to

Wasmtime GitHub notifications bot (May 19 2021 at 23:30):

peterhuene created PR review comment:

:tada: :tada: :tada:

Wasmtime GitHub notifications bot (May 19 2021 at 23:30):

peterhuene submitted PR review.

Wasmtime GitHub notifications bot (May 19 2021 at 23:30):

peterhuene created PR review comment:

Nit: we can replace this (and below) with InstanceAllocationStrategy::pooling() for brevity.

Wasmtime GitHub notifications bot (May 19 2021 at 23:30):

peterhuene created PR review comment:

Nit:

        // NB it's important that this destructor does not access `self.data`. That is

Wasmtime GitHub notifications bot (May 19 2021 at 23:30):

peterhuene created PR review comment:

At first when I reviewed this change I was concerned over the lack of registering the host func trampoline like we were doing before, but then I reviewed the changes in store.rs and saw how the host func trampolines are registered now directly with the store :+1:.

Wasmtime GitHub notifications bot (May 19 2021 at 23:30):

peterhuene created PR review comment:

I read through this comment and the logic seems sound regarding the justification of marking our fiber-based futures as Send :+1:

Wasmtime GitHub notifications bot (May 19 2021 at 23:30):

peterhuene created PR review comment:

Could you help me pinpoint where in the C API the 64-bit requirement is placed on Stored<T>? I assume it's related to wasmtime_extern_union but I haven't been able to spot where the size assumption comes into play.

That said, this scheme makes sense to me assuming that requirement.

Wasmtime GitHub notifications bot (May 19 2021 at 23:30):

peterhuene created PR review comment:

Thanks for fixing this!

Looking over what I wrote originally, I don't think we should be storing offsets in InstancePool at all.

Instead InstancePool::initialize and this code here that is resetting the offsets with VMOffsets::new(size_of::<*const u8>() as u8, &*self.empty_module) to ensure the current instance offsets match the associated module (empty both to start and when the instance has been deallocated).

For InstancePool::Initialize we'd want to pass in the module and instance limits to set the capacity on the memory/table maps.

Wasmtime GitHub notifications bot (May 19 2021 at 23:32):

peterhuene edited PR review comment.

Wasmtime GitHub notifications bot (May 20 2021 at 14:22):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (May 20 2021 at 14:22):

alexcrichton created PR review comment:

Oh this was a weird one where memory.reset_guard_pages() required a mut binding on memory but that call only happens with uffd/linux, so other platforms would get a warning about an unused mut. To fix that I just unconditionally added a line that would require mut memory here regardless of the platform so this would compile everywhere without warnings.

Wasmtime GitHub notifications bot (May 20 2021 at 14:54):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (May 20 2021 at 14:54):

alexcrichton created PR review comment:

Whoops I completely forgot to come back to this test and update it apparently...

Wasmtime GitHub notifications bot (May 20 2021 at 15:01):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (May 20 2021 at 15:01):

alexcrichton created PR review comment:

Ok I talked with @pchickey about this and our conclusion is that eventually GuestPtr will probably get phased out entirely. Instead of GuestPtr we'll generate the wiggle traits with raw borrowed strings already. This means that the BorrowChecker here can live in a mutable location and won't need interior mutability, alleviating the need for this Mutex. That'll happen in a future PR though

Wasmtime GitHub notifications bot (May 20 2021 at 15:07):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (May 20 2021 at 15:07):

alexcrichton created PR review comment:

The main requirement is that in the C API I would like to avoid types like wasmtime_func_t being owned types. Instead I'd like them to be POD-like with no destructor. For ergonomics they're also currently passed by-value everywhere in functions, e.g. returning wasmtime_func_t or taking wasmtime_func_t func or something like that. By-value structs always give me the willies (and I think concretely FFI interfaces like Python's ctypes doesn't support them) so that limited us to integers. The biggest integer for that was 64-bits, hence the 64-bit restriction.

There's other ways to change this though:

• With my idea above the 64-bits in the C API are just index bits, so nothing would be part of the store bits. Then Rust could use 128 bits and C could use 64.
• Another possibility is having the C API use 128-bit integers. This would probably show up as struct wasmtime_func_t { uint64_t hi; uint64_t low; } instead of a 128-bit integer, and then all APIs would take pointers to the struct (it'd never be returned by value)
• Alternatively we could go back to wasmtime_func_t* and require dtors, but I would personally prefer to avoid this.

While I originally felt that it was kinda nice to have 64-bit types in the C API, I may be coming around to the 128-bit-size-struct idea. That keeps safety in both Rust & C (somewhat at least) and I think shouldn't make bindings really that much more difficult. I think the ergonomics of the C bindings will go down a bit but that's not really the end of the world per se.

Do you think it's worth these shenanigans to get 64-bit types? Or should we just go ahead and use 128-bits everywhere?

Wasmtime GitHub notifications bot (May 20 2021 at 15:07):

alexcrichton updated PR #2897 from new-api to main.

Wasmtime GitHub notifications bot (May 20 2021 at 16:36):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (May 20 2021 at 16:36):

alexcrichton created PR review comment:

Hm so having thought a bit more about this this is the diff of "what if all the types in the wasmtime API were 128-bit values". That actually looks relatively clean to me and makes me think that I should probably just change to that? The Go/Python bindings had no trouble updating to using that idiom either.

This also means that if we ever want to store more stuff in there it shouldn't really be a problem either?

Wasmtime GitHub notifications bot (May 20 2021 at 16:38):

alexcrichton updated PR #2897 from new-api to main.

Wasmtime GitHub notifications bot (May 20 2021 at 18:41):

alexcrichton updated PR #2897 from new-api to main.

Wasmtime GitHub notifications bot (May 20 2021 at 18:50):

peterhuene submitted PR review.

Wasmtime GitHub notifications bot (May 20 2021 at 18:50):

peterhuene created PR review comment:

Ah, that makes complete sense.

Wasmtime GitHub notifications bot (May 20 2021 at 18:54):

peterhuene submitted PR review.

Wasmtime GitHub notifications bot (May 20 2021 at 19:00):

peterhuene submitted PR review.

Wasmtime GitHub notifications bot (May 20 2021 at 19:00):

peterhuene created PR review comment:

I think I would lean towards making that change so that we don't have to place limits on the number of stores that can be created (even if the limit seems reasonable today).

I looked over that commit and it looks good (minor spelling error on "belogns" in a few places, though).

Wasmtime GitHub notifications bot (May 20 2021 at 19:01):

pchickey submitted PR review.

Wasmtime GitHub notifications bot (May 20 2021 at 19:23):

alexcrichton updated PR #2897 from new-api to main.

Wasmtime GitHub notifications bot (May 20 2021 at 19:41):

alexcrichton updated PR #2897 from new-api to main.

Wasmtime GitHub notifications bot (May 20 2021 at 19:42):

alexcrichton updated PR #2897 from new-api to main.

Wasmtime GitHub notifications bot (May 20 2021 at 20:32):

alexcrichton updated PR #2897 from new-api to main.

Wasmtime GitHub notifications bot (May 20 2021 at 20:42):

alexcrichton updated PR #2897 from new-api to main.

Wasmtime GitHub notifications bot (May 20 2021 at 22:24):

alexcrichton updated PR #2897 from new-api to main.

Wasmtime GitHub notifications bot (May 21 2021 at 14:54):

alexcrichton updated PR #2897 from new-api to main.

Wasmtime GitHub notifications bot (May 21 2021 at 14:54):

alexcrichton updated PR #2897 from new-api to main.

Wasmtime GitHub notifications bot (May 21 2021 at 15:15):

alexcrichton updated PR #2897 from new-api to main.

Wasmtime GitHub notifications bot (May 21 2021 at 15:20):

alexcrichton updated PR #2897 from new-api to main.

Wasmtime GitHub notifications bot (May 21 2021 at 15:53):

alexcrichton updated PR #2897 from new-api to main.

Wasmtime GitHub notifications bot (May 21 2021 at 16:00):

alexcrichton updated PR #2897 from new-api to main.

Wasmtime GitHub notifications bot (May 21 2021 at 18:07):

alexcrichton updated PR #2897 from new-api to main.

Wasmtime GitHub notifications bot (May 21 2021 at 18:45):

alexcrichton updated PR #2897 from new-api to main.

Wasmtime GitHub notifications bot (May 21 2021 at 19:41):

alexcrichton updated PR #2897 from new-api to main.

Wasmtime GitHub notifications bot (May 21 2021 at 22:08):

alexcrichton updated PR #2897 from new-api to main.

Wasmtime GitHub notifications bot (May 22 2021 at 05:14):

alexcrichton updated PR #2897 from new-api to main.

Wasmtime GitHub notifications bot (May 22 2021 at 16:14):

alexcrichton updated PR #2897 from new-api to main.

Wasmtime GitHub notifications bot (May 22 2021 at 16:40):

alexcrichton updated PR #2897 from new-api to main.

Wasmtime GitHub notifications bot (May 28 2021 at 15:34):

alexcrichton updated PR #2897 from new-api to main.

Wasmtime GitHub notifications bot (May 28 2021 at 15:37):

alexcrichton updated PR #2897 from new-api to main.

Wasmtime GitHub notifications bot (Jun 01 2021 at 16:03):

bnjbvr submitted PR review.

Wasmtime GitHub notifications bot (Jun 01 2021 at 16:03):

bnjbvr created PR review comment:

Could we set default-features to false here, please? Otherwise this results in the default feature of wasmtime being forcefully enabled (because of cargo resolver's feature unification), and making it impossible to disable it in embeddings that indirectly make use of wiggle.

wasmtime = { path = "../wasmtime", version = "0.27.0", optional = true, default-features = false }

Wasmtime GitHub notifications bot (Jun 01 2021 at 16:05):

bnjbvr submitted PR review.

Wasmtime GitHub notifications bot (Jun 01 2021 at 16:05):

bnjbvr created PR review comment:

Hmm this is slightly wrong: should also re-enable manually the async feature.

Wasmtime GitHub notifications bot (Jun 01 2021 at 16:05):

bnjbvr edited PR review comment.

Wasmtime GitHub notifications bot (Jun 01 2021 at 16:05):

bnjbvr deleted PR review comment.

Wasmtime GitHub notifications bot (Jun 01 2021 at 17:24):

pchickey updated PR #2897 from new-api to main.

Wasmtime GitHub notifications bot (Jun 01 2021 at 17:24):

pchickey submitted PR review.

Wasmtime GitHub notifications bot (Jun 01 2021 at 17:24):

pchickey created PR review comment:

Thanks, I missed that.

Wasmtime GitHub notifications bot (Jun 01 2021 at 21:02):

alexcrichton updated PR #2897 from new-api to main.

Wasmtime GitHub notifications bot (Jun 01 2021 at 21:03):

alexcrichton updated PR #2897 from new-api to main.

Wasmtime GitHub notifications bot (Jun 01 2021 at 21:32):

alexcrichton updated PR #2897 from new-api to main.

Wasmtime GitHub notifications bot (Jun 03 2021 at 14:10):

alexcrichton merged PR #2897.

Last updated: Oct 23 2024 at 20:03 UTC