Stream: git-wasmtime

Topic: wasmtime / PR #1329 [wasi-common] add custom FdPool conta...


view this post on Zulip Wasmtime GitHub notifications bot (Mar 17 2020 at 09:10):

kubkon edited PR #1329 from fd_handles to master:

This PR adds a custom FdSet container which is intended
for use in wasi-common to track WASI fd allocs/deallocs. The
main aim for this container is to abstract away the current
approach of spawning new handles

fd = fd.checked_add(1).ok_or(...)?;

and to make it possible to reuse unused/reclaimed handles
which currently is not done.

The struct offers 3 methods to manage its functionality:

When figuring out the internals, I've tried to optimise for both
alloc and dealloc performance, and I believe we've got an amortised
O(1)~* performance for both (if my maths is right, and it may very
well not be, so please verify!).

In order to keep FdSet fairly generic, I've made sure not to hard-code
it for the current type system generated by wig (i.e., wasi::__wasi_fd_t
representing WASI handle), but rather, any type which wants to be managed
by FdSet needs to conform to Fd trait. This trait is quite simple as
it only requires a couple of rudimentary traits (although std:#️⃣:Hash
is quite a powerful assumption here!), and a custom method

Fd::next(&self) -> Option<Self>;

which is there to encapsulate creating another handle from the given one.
In the current state of the code, that'd be simply u32::checked_add(1).
When wiggle makes it way into the wasi-common, I'd imagine it being
similar to

fn next(&self) -> Option<Self> {
    self.0.checked_add(1).map(Self::from)
}

Anyhow, I'd be happy to learn your thoughts about this design!

view this post on Zulip Wasmtime GitHub notifications bot (Mar 17 2020 at 09:11):

kubkon edited PR #1329 from fd_handles to master:

This PR adds a custom FdPool container which is intended
for use in wasi-common to track WASI fd allocs/deallocs. The
main aim for this container is to abstract away the current
approach of spawning new handles

fd = fd.checked_add(1).ok_or(...)?;

and to make it possible to reuse unused/reclaimed handles
which currently is not done.

The struct offers 3 methods to manage its functionality:

When figuring out the internals, I've tried to optimise for both
alloc and dealloc performance, and I believe we've got an amortised
O(1)~* performance for both (if my maths is right, and it may very
well not be, so please verify!).

In order to keep FdPool fairly generic, I've made sure not to hard-code
it for the current type system generated by wig (i.e., wasi::__wasi_fd_t
representing WASI handle), but rather, any type which wants to be managed
by FdPool needs to conform to Fd trait. This trait is quite simple as
it only requires a couple of rudimentary traits (although std:#️⃣:Hash
is quite a powerful assumption here!), and a custom method

Fd::next(&self) -> Option<Self>;

which is there to encapsulate creating another handle from the given one.
In the current state of the code, that'd be simply u32::checked_add(1).
When wiggle makes it way into the wasi-common, I'd imagine it being
similar to

fn next(&self) -> Option<Self> {
    self.0.checked_add(1).map(Self::from)
}

Anyhow, I'd be happy to learn your thoughts about this design!

view this post on Zulip Wasmtime GitHub notifications bot (Mar 17 2020 at 14:49):

alexcrichton submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 17 2020 at 14:49):

alexcrichton created PR Review Comment:

I think we can probably avoid a literal list-of-indices here perhaps? We could have a Vec of available indices, as well as a "next index to hand out" stored as Option<Fd>. Allocation would pop from the Vec or otherwise .take() the next index. If the next index was taken we store the result of .next() back in there.

Later during deallocate we'd just push the entry onto the Vec.

I think that may simplify a bit here and make it a bit more lightweight in terms of overhead?

view this post on Zulip Wasmtime GitHub notifications bot (Mar 17 2020 at 14:49):

alexcrichton created PR Review Comment:

Instead of .replace I think this could be self.stdin = Some(...), right?

view this post on Zulip Wasmtime GitHub notifications bot (Mar 17 2020 at 14:49):

alexcrichton submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 17 2020 at 14:49):

alexcrichton created PR Review Comment:

Could this file reexport from the current snapshot to avoid duplication?

view this post on Zulip Wasmtime GitHub notifications bot (Mar 17 2020 at 15:11):

kubkon submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 17 2020 at 15:11):

kubkon created PR Review Comment:

So basically, you suggest to skip the preallocation step, did I get it right? If so, this is what I originally had, just thought we could preallocate a couple upfront as means of optimisation but I guess that might have been somewhat premature ;-)

view this post on Zulip Wasmtime GitHub notifications bot (Mar 17 2020 at 15:13):

alexcrichton submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 17 2020 at 15:13):

alexcrichton created PR Review Comment:

Right yeah I think the preallocation can largely be skipped. We can always continue to optimize it more though if necessary!

view this post on Zulip Wasmtime GitHub notifications bot (Mar 17 2020 at 15:14):

kubkon submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 17 2020 at 15:14):

kubkon created PR Review Comment:

Excellent question! I thought about this myself, and in principle, I'd love if we could. The problem is with type aliasing between the two snapshots. The compiler gets confused which wasi::__wasi_fd_t as Fd to use. Do you think there's a way to overcome this? Would perhaps type aliasing work here?

view this post on Zulip Wasmtime GitHub notifications bot (Mar 17 2020 at 15:14):

kubkon submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 17 2020 at 15:14):

kubkon created PR Review Comment:

Agreed! Let me roll it back then!

view this post on Zulip Wasmtime GitHub notifications bot (Mar 17 2020 at 15:17):

alexcrichton submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 17 2020 at 15:17):

alexcrichton created PR Review Comment:

Hm I'm not sure I understand the compiler error, wanna push up a branch I can poke at?

view this post on Zulip Wasmtime GitHub notifications bot (Mar 17 2020 at 15:42):

kubkon updated PR #1329 from fd_handles to master:

This PR adds a custom FdPool container which is intended
for use in wasi-common to track WASI fd allocs/deallocs. The
main aim for this container is to abstract away the current
approach of spawning new handles

fd = fd.checked_add(1).ok_or(...)?;

and to make it possible to reuse unused/reclaimed handles
which currently is not done.

The struct offers 3 methods to manage its functionality:

When figuring out the internals, I've tried to optimise for both
alloc and dealloc performance, and I believe we've got an amortised
O(1)~* performance for both (if my maths is right, and it may very
well not be, so please verify!).

In order to keep FdPool fairly generic, I've made sure not to hard-code
it for the current type system generated by wig (i.e., wasi::__wasi_fd_t
representing WASI handle), but rather, any type which wants to be managed
by FdPool needs to conform to Fd trait. This trait is quite simple as
it only requires a couple of rudimentary traits (although std:#️⃣:Hash
is quite a powerful assumption here!), and a custom method

Fd::next(&self) -> Option<Self>;

which is there to encapsulate creating another handle from the given one.
In the current state of the code, that'd be simply u32::checked_add(1).
When wiggle makes it way into the wasi-common, I'd imagine it being
similar to

fn next(&self) -> Option<Self> {
    self.0.checked_add(1).map(Self::from)
}

Anyhow, I'd be happy to learn your thoughts about this design!

view this post on Zulip Wasmtime GitHub notifications bot (Mar 17 2020 at 15:53):

kubkon submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 17 2020 at 15:53):

kubkon created PR Review Comment:

Right, I now remembered what the issue was. Since __wasi_fd_t is a type alias to u32 in both snapshots, we obviously cannot define two implementations of the form

// this one is in snapshot1, in wasi.rs
impl crate::fdpool::Fd for __wasi_fd_t {
   // ...
}

and

// this one is in snapshot0, in old/snapshot0/wasi.rs
impl crate::fdpool::Fd for __wasi_fd_t {
    // ...
}

I guess given that there is virtually no ABI difference between the two types across both snapshots, we could simply ignore implementing it in snapshot0. Would that be acceptable? I mean given that soon-ish we'd want to use wiggle to completely rethink how we handle multiple snapshots with as little code duplication as possible, this seems fair to me.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 17 2020 at 16:13):

alexcrichton submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 17 2020 at 16:13):

alexcrichton created PR Review Comment:

Yeah I think that'll be alright!

view this post on Zulip Wasmtime GitHub notifications bot (Mar 17 2020 at 16:14):

alexcrichton submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 17 2020 at 16:14):

alexcrichton created PR Review Comment:

This could use ? I think if you wanted to be super nifty

view this post on Zulip Wasmtime GitHub notifications bot (Mar 17 2020 at 16:16):

kubkon updated PR #1329 from fd_handles to master:

This PR adds a custom FdPool container which is intended
for use in wasi-common to track WASI fd allocs/deallocs. The
main aim for this container is to abstract away the current
approach of spawning new handles

fd = fd.checked_add(1).ok_or(...)?;

and to make it possible to reuse unused/reclaimed handles
which currently is not done.

The struct offers 3 methods to manage its functionality:

When figuring out the internals, I've tried to optimise for both
alloc and dealloc performance, and I believe we've got an amortised
O(1)~* performance for both (if my maths is right, and it may very
well not be, so please verify!).

In order to keep FdPool fairly generic, I've made sure not to hard-code
it for the current type system generated by wig (i.e., wasi::__wasi_fd_t
representing WASI handle), but rather, any type which wants to be managed
by FdPool needs to conform to Fd trait. This trait is quite simple as
it only requires a couple of rudimentary traits (although std:#️⃣:Hash
is quite a powerful assumption here!), and a custom method

Fd::next(&self) -> Option<Self>;

which is there to encapsulate creating another handle from the given one.
In the current state of the code, that'd be simply u32::checked_add(1).
When wiggle makes it way into the wasi-common, I'd imagine it being
similar to

fn next(&self) -> Option<Self> {
    self.0.checked_add(1).map(Self::from)
}

Anyhow, I'd be happy to learn your thoughts about this design!

view this post on Zulip Wasmtime GitHub notifications bot (Mar 17 2020 at 16:17):

kubkon submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 17 2020 at 16:17):

kubkon created PR Review Comment:

Of course, well spotted, thanks!

view this post on Zulip Wasmtime GitHub notifications bot (Mar 17 2020 at 16:18):

kubkon updated PR #1329 from fd_handles to master:

This PR adds a custom FdPool container which is intended
for use in wasi-common to track WASI fd allocs/deallocs. The
main aim for this container is to abstract away the current
approach of spawning new handles

fd = fd.checked_add(1).ok_or(...)?;

and to make it possible to reuse unused/reclaimed handles
which currently is not done.

The struct offers 3 methods to manage its functionality:

When figuring out the internals, I've tried to optimise for both
alloc and dealloc performance, and I believe we've got an amortised
O(1)~* performance for both (if my maths is right, and it may very
well not be, so please verify!).

In order to keep FdPool fairly generic, I've made sure not to hard-code
it for the current type system generated by wig (i.e., wasi::__wasi_fd_t
representing WASI handle), but rather, any type which wants to be managed
by FdPool needs to conform to Fd trait. This trait is quite simple as
it only requires a couple of rudimentary traits (although std:#️⃣:Hash
is quite a powerful assumption here!), and a custom method

Fd::next(&self) -> Option<Self>;

which is there to encapsulate creating another handle from the given one.
In the current state of the code, that'd be simply u32::checked_add(1).
When wiggle makes it way into the wasi-common, I'd imagine it being
similar to

fn next(&self) -> Option<Self> {
    self.0.checked_add(1).map(Self::from)
}

Anyhow, I'd be happy to learn your thoughts about this design!

view this post on Zulip Wasmtime GitHub notifications bot (Mar 17 2020 at 16:21):

kubkon requested alexcrichton, pchickey, and sunfishcode for a review on PR #1329.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 17 2020 at 16:40):

alexcrichton submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 17 2020 at 17:06):

sunfishcode submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 17 2020 at 17:06):

sunfishcode submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 17 2020 at 17:06):

sunfishcode created PR Review Comment:

This can use a slice literal, &[ ... ], instead of a temporary vec!.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 17 2020 at 17:06):

sunfishcode created PR Review Comment:

In place of the claimed check (which I propose removing above), this can instead assert that fd is not greater than next_alloc, and debug_assert that it's not in available (debug_assert because it could be slow in some cases).

view this post on Zulip Wasmtime GitHub notifications bot (Mar 17 2020 at 17:06):

sunfishcode created PR Review Comment:

As above, this can use a slice literal instead of a temporary vec!.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 17 2020 at 17:06):

sunfishcode created PR Review Comment:

For tidiness, we should do the entries.remove before doing the fds.deallocate, so that we remove things in reverse order from how we add them, so that we maintain an invariant that at any time a fd is in entries, it's allocated in fds too.

With that, we can remove the claimed field of FdPool, and assume that removal always succeeds.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 17 2020 at 17:37):

kubkon submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 17 2020 at 17:37):

kubkon created PR Review Comment:

The reason I didn't use slice literal here is due to the dereferencing rules. In PendingEntry::File arm we use OsHandle::from which requires std::fs::File, and with slice literal we provide a ref instead.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 17 2020 at 17:38):

kubkon submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 17 2020 at 17:38):

kubkon created PR Review Comment:

See above.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 17 2020 at 17:40):

kubkon submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 17 2020 at 17:40):

kubkon created PR Review Comment:

Hmm, I'm fine with asserts. However, the reason I've designed deallocate return bool was to mimic the behaviour of HashSet::remove which returns a bool to signal if removal worked or not (i.e., if any element was actually removed in the end). In this case, I guess you're right that if we try removing from the claimed HashSet and it actually fails, then something went horribly wrong as it shouldn't ever happen.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 17 2020 at 17:40):

kubkon submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 17 2020 at 17:40):

kubkon created PR Review Comment:

Ah, good call, thanks!

view this post on Zulip Wasmtime GitHub notifications bot (Mar 17 2020 at 17:43):

sunfishcode submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 17 2020 at 17:43):

sunfishcode created PR Review Comment:

Ah, makes sense!

view this post on Zulip Wasmtime GitHub notifications bot (Mar 17 2020 at 17:44):

kubkon created PR Review Comment:

Oh, after re-reading your comment again, checking whether fd is not greater than next_alloc will require an if anyway since next_alloc is wrapped in an Option and it will not always be guaranteed to be Some (in particular, when we exceed all possible allocs for instance, e.g., std::u32::MAX).

view this post on Zulip Wasmtime GitHub notifications bot (Mar 17 2020 at 17:44):

kubkon submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 17 2020 at 17:45):

kubkon submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 17 2020 at 17:45):

kubkon created PR Review Comment:

Given the above, unless you can think of a cleaner way of doing it, I guess I'd be in favour of the original approach. What do you think?

view this post on Zulip Wasmtime GitHub notifications bot (Mar 17 2020 at 17:50):

kubkon updated PR #1329 from fd_handles to master:

This PR adds a custom FdPool container which is intended
for use in wasi-common to track WASI fd allocs/deallocs. The
main aim for this container is to abstract away the current
approach of spawning new handles

fd = fd.checked_add(1).ok_or(...)?;

and to make it possible to reuse unused/reclaimed handles
which currently is not done.

The struct offers 3 methods to manage its functionality:

When figuring out the internals, I've tried to optimise for both
alloc and dealloc performance, and I believe we've got an amortised
O(1)~* performance for both (if my maths is right, and it may very
well not be, so please verify!).

In order to keep FdPool fairly generic, I've made sure not to hard-code
it for the current type system generated by wig (i.e., wasi::__wasi_fd_t
representing WASI handle), but rather, any type which wants to be managed
by FdPool needs to conform to Fd trait. This trait is quite simple as
it only requires a couple of rudimentary traits (although std:#️⃣:Hash
is quite a powerful assumption here!), and a custom method

Fd::next(&self) -> Option<Self>;

which is there to encapsulate creating another handle from the given one.
In the current state of the code, that'd be simply u32::checked_add(1).
When wiggle makes it way into the wasi-common, I'd imagine it being
similar to

fn next(&self) -> Option<Self> {
    self.0.checked_add(1).map(Self::from)
}

Anyhow, I'd be happy to learn your thoughts about this design!

view this post on Zulip Wasmtime GitHub notifications bot (Mar 17 2020 at 17:51):

kubkon created PR Review Comment:

Done in 647cc1f. I'm still a little bit unclear how to address FdPool::deallocate though. Would you mind giving it another look after re-reading my comments please?

view this post on Zulip Wasmtime GitHub notifications bot (Mar 17 2020 at 17:51):

kubkon submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 17 2020 at 17:54):

kubkon created PR Review Comment:

Perhaps we could assert! on self.claimed.remove instead? This would ensure we panic! if the client code ever provides an illegal fd value?

view this post on Zulip Wasmtime GitHub notifications bot (Mar 17 2020 at 17:54):

kubkon submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 17 2020 at 20:08):

kubkon updated PR #1329 from fd_handles to master:

This PR adds a custom FdPool container which is intended
for use in wasi-common to track WASI fd allocs/deallocs. The
main aim for this container is to abstract away the current
approach of spawning new handles

fd = fd.checked_add(1).ok_or(...)?;

and to make it possible to reuse unused/reclaimed handles
which currently is not done.

The struct offers 3 methods to manage its functionality:

When figuring out the internals, I've tried to optimise for both
alloc and dealloc performance, and I believe we've got an amortised
O(1)~* performance for both (if my maths is right, and it may very
well not be, so please verify!).

In order to keep FdPool fairly generic, I've made sure not to hard-code
it for the current type system generated by wig (i.e., wasi::__wasi_fd_t
representing WASI handle), but rather, any type which wants to be managed
by FdPool needs to conform to Fd trait. This trait is quite simple as
it only requires a couple of rudimentary traits (although std:#️⃣:Hash
is quite a powerful assumption here!), and a custom method

Fd::next(&self) -> Option<Self>;

which is there to encapsulate creating another handle from the given one.
In the current state of the code, that'd be simply u32::checked_add(1).
When wiggle makes it way into the wasi-common, I'd imagine it being
similar to

fn next(&self) -> Option<Self> {
    self.0.checked_add(1).map(Self::from)
}

Anyhow, I'd be happy to learn your thoughts about this design!

view this post on Zulip Wasmtime GitHub notifications bot (Mar 17 2020 at 20:13):

kubkon requested alexcrichton, and pchickey for a review on PR #1329.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 17 2020 at 20:13):

kubkon requested alexcrichton, pchickey, and sunfishcode for a review on PR #1329.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 17 2020 at 20:20):

alexcrichton submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 17 2020 at 20:25):

sunfishcode submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 17 2020 at 21:58):

kubkon merged PR #1329.


Last updated: Dec 23 2024 at 12:05 UTC