Stream: git-wasmtime

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

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:

• FdSet::new initialises the internal data structures,
  and most notably, it preallocates an FdSet::BATCH_SIZE
  worth of handles in such a way that we always start popping
  from the "smallest" handle (think of it as of reversed stack,
  I guess; it's not a binary heap since we don't really care
  whether internally the handles are sorted in some way, just that
  the "largets" handle is at the bottom. Why will become clear
  when describing allocate method.)

• FdSet::allocate pops the next available handle if one is available.
  The tricky bit here is that, if we run out of handles, we preallocate
  the next FdSet::BATCH_SIZE worth of handles starting from the
  latest popped handle (i.e., the "largest" handle). This
  works only because we make sure to only ever pop and push already
  existing handles from the back, and push _new_ handles (from the
  preallocation step) from the front. When we ultimately run out
  of _all_ available handles, we then return None for the client
  to handle in some way (e.g., throwing an error such as WasiError::EMFILE
  or whatnot).

• FdSet::deallocate returns the already allocated handle back to
  the pool for further reuse.

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!

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:

• FdPool::new initialises the internal data structures,
  and most notably, it preallocates an FdPool::BATCH_SIZE
  worth of handles in such a way that we always start popping
  from the "smallest" handle (think of it as of reversed stack,
  I guess; it's not a binary heap since we don't really care
  whether internally the handles are sorted in some way, just that
  the "largets" handle is at the bottom. Why will become clear
  when describing allocate method.)

• FdPool::allocate pops the next available handle if one is available.
  The tricky bit here is that, if we run out of handles, we preallocate
  the next FdPool::BATCH_SIZE worth of handles starting from the
  latest popped handle (i.e., the "largest" handle). This
  works only because we make sure to only ever pop and push already
  existing handles from the back, and push _new_ handles (from the
  preallocation step) from the front. When we ultimately run out
  of _all_ available handles, we then return None for the client
  to handle in some way (e.g., throwing an error such as WasiError::EMFILE
  or whatnot).

• FdPool::deallocate returns the already allocated handle back to
  the pool for further reuse.

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!

Wasmtime GitHub notifications bot (Mar 17 2020 at 14:49):

alexcrichton submitted PR Review.

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?

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?

Wasmtime GitHub notifications bot (Mar 17 2020 at 14:49):

alexcrichton submitted PR Review.

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?

Wasmtime GitHub notifications bot (Mar 17 2020 at 15:11):

kubkon submitted PR Review.

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 ;-)

Wasmtime GitHub notifications bot (Mar 17 2020 at 15:13):

alexcrichton submitted PR Review.

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!

Wasmtime GitHub notifications bot (Mar 17 2020 at 15:14):

kubkon submitted PR Review.

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?

Wasmtime GitHub notifications bot (Mar 17 2020 at 15:14):

kubkon submitted PR Review.

Wasmtime GitHub notifications bot (Mar 17 2020 at 15:14):

kubkon created PR Review Comment:

Agreed! Let me roll it back then!

Wasmtime GitHub notifications bot (Mar 17 2020 at 15:17):

alexcrichton submitted PR Review.

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?

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:

• FdPool::new initialises the internal data structures,
  and most notably, it preallocates an FdPool::BATCH_SIZE
  worth of handles in such a way that we always start popping
  from the "smallest" handle (think of it as of reversed stack,
  I guess; it's not a binary heap since we don't really care
  whether internally the handles are sorted in some way, just that
  the "largets" handle is at the bottom. Why will become clear
  when describing allocate method.)

• FdPool::allocate pops the next available handle if one is available.
  The tricky bit here is that, if we run out of handles, we preallocate
  the next FdPool::BATCH_SIZE worth of handles starting from the
  latest popped handle (i.e., the "largest" handle). This
  works only because we make sure to only ever pop and push already
  existing handles from the back, and push _new_ handles (from the
  preallocation step) from the front. When we ultimately run out
  of _all_ available handles, we then return None for the client
  to handle in some way (e.g., throwing an error such as WasiError::EMFILE
  or whatnot).

• FdPool::deallocate returns the already allocated handle back to
  the pool for further reuse.

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!

Wasmtime GitHub notifications bot (Mar 17 2020 at 15:53):

kubkon submitted PR Review.

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.

Wasmtime GitHub notifications bot (Mar 17 2020 at 16:13):

alexcrichton submitted PR Review.

Wasmtime GitHub notifications bot (Mar 17 2020 at 16:13):

alexcrichton created PR Review Comment:

Yeah I think that'll be alright!

Wasmtime GitHub notifications bot (Mar 17 2020 at 16:14):

alexcrichton submitted PR Review.

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

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:

• FdPool::new initialises the internal data structures,
  and most notably, it preallocates an FdPool::BATCH_SIZE
  worth of handles in such a way that we always start popping
  from the "smallest" handle (think of it as of reversed stack,
  I guess; it's not a binary heap since we don't really care
  whether internally the handles are sorted in some way, just that
  the "largets" handle is at the bottom. Why will become clear
  when describing allocate method.)

• FdPool::allocate pops the next available handle if one is available.
  The tricky bit here is that, if we run out of handles, we preallocate
  the next FdPool::BATCH_SIZE worth of handles starting from the
  latest popped handle (i.e., the "largest" handle). This
  works only because we make sure to only ever pop and push already
  existing handles from the back, and push _new_ handles (from the
  preallocation step) from the front. When we ultimately run out
  of _all_ available handles, we then return None for the client
  to handle in some way (e.g., throwing an error such as WasiError::EMFILE
  or whatnot).

• FdPool::deallocate returns the already allocated handle back to
  the pool for further reuse.

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!

Wasmtime GitHub notifications bot (Mar 17 2020 at 16:17):

kubkon submitted PR Review.

Wasmtime GitHub notifications bot (Mar 17 2020 at 16:17):

kubkon created PR Review Comment:

Of course, well spotted, thanks!

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:

• FdPool::new initialises the internal data structures,
  and most notably, it preallocates an FdPool::BATCH_SIZE
  worth of handles in such a way that we always start popping
  from the "smallest" handle (think of it as of reversed stack,
  I guess; it's not a binary heap since we don't really care
  whether internally the handles are sorted in some way, just that
  the "largets" handle is at the bottom. Why will become clear
  when describing allocate method.)

• FdPool::allocate pops the next available handle if one is available.
  The tricky bit here is that, if we run out of handles, we preallocate
  the next FdPool::BATCH_SIZE worth of handles starting from the
  latest popped handle (i.e., the "largest" handle). This
  works only because we make sure to only ever pop and push already
  existing handles from the back, and push _new_ handles (from the
  preallocation step) from the front. When we ultimately run out
  of _all_ available handles, we then return None for the client
  to handle in some way (e.g., throwing an error such as WasiError::EMFILE
  or whatnot).

• FdPool::deallocate returns the already allocated handle back to
  the pool for further reuse.

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!

Wasmtime GitHub notifications bot (Mar 17 2020 at 16:21):

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

Wasmtime GitHub notifications bot (Mar 17 2020 at 16:40):

alexcrichton submitted PR Review.

Wasmtime GitHub notifications bot (Mar 17 2020 at 17:06):

sunfishcode submitted PR Review.

Wasmtime GitHub notifications bot (Mar 17 2020 at 17:06):

sunfishcode submitted PR Review.

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!.

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).

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!.

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.

Wasmtime GitHub notifications bot (Mar 17 2020 at 17:37):

kubkon submitted PR Review.

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.

Wasmtime GitHub notifications bot (Mar 17 2020 at 17:38):

kubkon submitted PR Review.

Wasmtime GitHub notifications bot (Mar 17 2020 at 17:38):

kubkon created PR Review Comment:

See above.

Wasmtime GitHub notifications bot (Mar 17 2020 at 17:40):

kubkon submitted PR Review.

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.

Wasmtime GitHub notifications bot (Mar 17 2020 at 17:40):

kubkon submitted PR Review.

Wasmtime GitHub notifications bot (Mar 17 2020 at 17:40):

kubkon created PR Review Comment:

Ah, good call, thanks!

Wasmtime GitHub notifications bot (Mar 17 2020 at 17:43):

sunfishcode submitted PR Review.

Wasmtime GitHub notifications bot (Mar 17 2020 at 17:43):

sunfishcode created PR Review Comment:

Ah, makes sense!

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).

Wasmtime GitHub notifications bot (Mar 17 2020 at 17:44):

kubkon submitted PR Review.

Wasmtime GitHub notifications bot (Mar 17 2020 at 17:45):

kubkon submitted PR Review.

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?

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:

• FdPool::new initialises the internal data structures,
  and most notably, it preallocates an FdPool::BATCH_SIZE
  worth of handles in such a way that we always start popping
  from the "smallest" handle (think of it as of reversed stack,
  I guess; it's not a binary heap since we don't really care
  whether internally the handles are sorted in some way, just that
  the "largets" handle is at the bottom. Why will become clear
  when describing allocate method.)

• FdPool::allocate pops the next available handle if one is available.
  The tricky bit here is that, if we run out of handles, we preallocate
  the next FdPool::BATCH_SIZE worth of handles starting from the
  latest popped handle (i.e., the "largest" handle). This
  works only because we make sure to only ever pop and push already
  existing handles from the back, and push _new_ handles (from the
  preallocation step) from the front. When we ultimately run out
  of _all_ available handles, we then return None for the client
  to handle in some way (e.g., throwing an error such as WasiError::EMFILE
  or whatnot).

• FdPool::deallocate returns the already allocated handle back to
  the pool for further reuse.

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!

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?

Wasmtime GitHub notifications bot (Mar 17 2020 at 17:51):

kubkon submitted PR Review.

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?

Wasmtime GitHub notifications bot (Mar 17 2020 at 17:54):

kubkon submitted PR Review.

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:

• FdPool::new initialises the internal data structures,
  and most notably, it preallocates an FdPool::BATCH_SIZE
  worth of handles in such a way that we always start popping
  from the "smallest" handle (think of it as of reversed stack,
  I guess; it's not a binary heap since we don't really care
  whether internally the handles are sorted in some way, just that
  the "largets" handle is at the bottom. Why will become clear
  when describing allocate method.)

• FdPool::allocate pops the next available handle if one is available.
  The tricky bit here is that, if we run out of handles, we preallocate
  the next FdPool::BATCH_SIZE worth of handles starting from the
  latest popped handle (i.e., the "largest" handle). This
  works only because we make sure to only ever pop and push already
  existing handles from the back, and push _new_ handles (from the
  preallocation step) from the front. When we ultimately run out
  of _all_ available handles, we then return None for the client
  to handle in some way (e.g., throwing an error such as WasiError::EMFILE
  or whatnot).

• FdPool::deallocate returns the already allocated handle back to
  the pool for further reuse.

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!

Wasmtime GitHub notifications bot (Mar 17 2020 at 20:13):

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

Wasmtime GitHub notifications bot (Mar 17 2020 at 20:13):

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

Wasmtime GitHub notifications bot (Mar 17 2020 at 20:20):

alexcrichton submitted PR Review.

Wasmtime GitHub notifications bot (Mar 17 2020 at 20:25):

sunfishcode submitted PR Review.

Wasmtime GitHub notifications bot (Mar 17 2020 at 21:58):

kubkon merged PR #1329.

Last updated: Nov 22 2024 at 16:03 UTC