Stream: git-wasmtime

Topic: wasmtime / PR #1412 Refactor use of Refs and RefMuts in w...

Wasmtime GitHub notifications bot (Mar 26 2020 at 12:09):

kubkon opened PR #1412 from rc-entry to master:

This commit refactors the use of Refs and RefMuts in wasi-common.
Now, Entry is stored behind an Rc inside the EntryTable. The Entry
itself on the other hand now stores rights behind a RefCell and the
descriptor as Rc<RefCell<..>> combo to enable easy reference tracking
and interior mutability which is required down the line in a couple of
syscalls. In essence, this implies that we no longer have need for
mutable accessor to Entry from WasiCtx, and so all related methods
go away (get_entry_mut, etc.).

While here, I've also simplified handling and aggregating of rights on
the Entry object. Instead of storing base and inheriting rights as
separate fields, they are now aggregated into one struct EntryRights
which features convenient constructors for each possible combination; i.e.,
when only base rights are set, or both base and inheriting are set, or
both are left as empty. Since we do need to be able to mutate those
rights down the line in fdstat_set_rights syscall, this object
is kept behind a RefCell (note no Rc since we don't need to pass it
around anywhere).

The descriptor field in Entry is now kept behind Rc<RefCell<..>> combo
since we not only need to mutate it down the line, but we also need to
be able to pass it around (as part of the machinery making poll_oneoff
work).

I've also removed as_file and try_clone methods on Descriptor struct
since they were adding more noise than necessary, and making them work
with Rc was unnecessarily complicated.

Finally, I've converted the get_dir_from_os_handle function into a
method attached to the OsHandle itself, called dir_stream. IMHO,
it makes more sense to have it there directly as a method than as a separate
function.

Wasmtime GitHub notifications bot (Mar 26 2020 at 12:09):

kubkon requested alexcrichton and sunfishcode for a review on PR #1412.

Wasmtime GitHub notifications bot (Mar 26 2020 at 12:09):

kubkon requested alexcrichton and sunfishcode for a review on PR #1412.

Wasmtime GitHub notifications bot (Mar 26 2020 at 12:14):

kubkon updated PR #1412 from rc-entry to master:

This commit refactors the use of Refs and RefMuts in wasi-common.
Now, Entry is stored behind an Rc inside the EntryTable. The Entry
itself on the other hand now stores rights behind a RefCell and the
descriptor as Rc<RefCell<..>> combo to enable easy reference tracking
and interior mutability which is required down the line in a couple of
syscalls. In essence, this implies that we no longer have need for
mutable accessor to Entry from WasiCtx, and so all related methods
go away (get_entry_mut, etc.).

While here, I've also simplified handling and aggregating of rights on
the Entry object. Instead of storing base and inheriting rights as
separate fields, they are now aggregated into one struct EntryRights
which features convenient constructors for each possible combination; i.e.,
when only base rights are set, or both base and inheriting are set, or
both are left as empty. Since we do need to be able to mutate those
rights down the line in fdstat_set_rights syscall, this object
is kept behind a RefCell (note no Rc since we don't need to pass it
around anywhere).

The descriptor field in Entry is now kept behind Rc<RefCell<..>> combo
since we not only need to mutate it down the line, but we also need to
be able to pass it around (as part of the machinery making poll_oneoff
work).

I've also removed as_file and try_clone methods on Descriptor struct
since they were adding more noise than necessary, and making them work
with Rc was unnecessarily complicated.

Finally, I've converted the get_dir_from_os_handle function into a
method attached to the OsHandle itself, called dir_stream. IMHO,
it makes more sense to have it there directly as a method than as a separate
function.

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

alexcrichton submitted PR Review.

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

alexcrichton submitted PR Review.

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

alexcrichton created PR Review Comment:

This is a Copy type I think, so could this perhaps be a Cell<types::Fdflags> to entirely elimitate the possibility of panics?

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

alexcrichton created PR Review Comment:

So to prefix this, I'm not very familiar with what this Descriptor is and what it's used for. That being said I think it'd be great, if possible, to continue to push the RefCell deeper here. The purpose of removing the ability to get &mut Entry was all about making mutable borrows as small as possible to eliminate possibilities of panics, and I think the same might apply here as well. It may even be possible to have Descriptor only have Cell fields, but ideally any RefCell fields would have very small borrows and no methods would have to return a Ref or RefMut

Wasmtime GitHub notifications bot (Mar 26 2020 at 19:55):

kubkon updated PR #1412 from rc-entry to master:

This commit refactors the use of Refs and RefMuts in wasi-common.
Now, Entry is stored behind an Rc inside the EntryTable. The Entry
itself on the other hand now stores rights behind a RefCell and the
descriptor as Rc<RefCell<..>> combo to enable easy reference tracking
and interior mutability which is required down the line in a couple of
syscalls. In essence, this implies that we no longer have need for
mutable accessor to Entry from WasiCtx, and so all related methods
go away (get_entry_mut, etc.).

While here, I've also simplified handling and aggregating of rights on
the Entry object. Instead of storing base and inheriting rights as
separate fields, they are now aggregated into one struct EntryRights
which features convenient constructors for each possible combination; i.e.,
when only base rights are set, or both base and inheriting are set, or
both are left as empty. Since we do need to be able to mutate those
rights down the line in fdstat_set_rights syscall, this object
is kept behind a RefCell (note no Rc since we don't need to pass it
around anywhere).

The descriptor field in Entry is now kept behind Rc<RefCell<..>> combo
since we not only need to mutate it down the line, but we also need to
be able to pass it around (as part of the machinery making poll_oneoff
work).

I've also removed as_file and try_clone methods on Descriptor struct
since they were adding more noise than necessary, and making them work
with Rc was unnecessarily complicated.

Finally, I've converted the get_dir_from_os_handle function into a
method attached to the OsHandle itself, called dir_stream. IMHO,
it makes more sense to have it there directly as a method than as a separate
function.

Wasmtime GitHub notifications bot (Mar 26 2020 at 20:18):

kubkon created PR Review Comment:

Yep, thanks!

Wasmtime GitHub notifications bot (Mar 26 2020 at 20:18):

kubkon submitted PR Review.

Wasmtime GitHub notifications bot (Mar 26 2020 at 20:54):

kubkon submitted PR Review.

Wasmtime GitHub notifications bot (Mar 26 2020 at 20:54):

kubkon created PR Review Comment:

I think you're right. Although making it work will require some bigger reorg to OsHandle especially on BSD platform. I have an idea for this and will investigate but given that this probably will be a bigger change, I'd prefer to have it as a subsequent PR. In short, if we make OsHandle a bit closer to the metal by replacing File with RawFd, we can make OsHandle Copy which would then make it possible to have Descriptor inside a Cell. There's a little bit more work involved with BSD since currently OsHandle, in addition to File, also stores an optional yanix::Dir (which is non-Copy since it's got a custom Drop impl) and it would probably make sense to split the handle into either representing an actual file, or a directory. Then, we wouldn't need Option nor interior mutability, since we could easily make yanix::Dir Copy calling it something like yanix::UnsafeDir and specifically mentioning that it is the user's responsibility to close the underlying dir stream with libc::closedir fn. Does that make sense?

Wasmtime GitHub notifications bot (Mar 26 2020 at 21:00):

kubkon submitted PR Review.

Wasmtime GitHub notifications bot (Mar 26 2020 at 21:00):

kubkon created PR Review Comment:

Oh, and the only reason we ever need to mutate the actual Descriptor in Entry is because on Windows, setting fdstat flags requires reopening the file.

Wasmtime GitHub notifications bot (Mar 26 2020 at 23:04):

alexcrichton submitted PR Review.

Wasmtime GitHub notifications bot (Mar 26 2020 at 23:04):

alexcrichton created PR Review Comment:

Ah ok punting this to later seems fine, in any case looks like a great refactoring!

Wasmtime GitHub notifications bot (Mar 26 2020 at 23:04):

alexcrichton submitted PR Review.

Wasmtime GitHub notifications bot (Mar 27 2020 at 08:00):

kubkon updated PR #1412 from rc-entry to master:

This commit refactors the use of Refs and RefMuts in wasi-common.
Now, Entry is stored behind an Rc inside the EntryTable. The Entry
itself on the other hand now stores rights behind a RefCell and the
descriptor as Rc<RefCell<..>> combo to enable easy reference tracking
and interior mutability which is required down the line in a couple of
syscalls. In essence, this implies that we no longer have need for
mutable accessor to Entry from WasiCtx, and so all related methods
go away (get_entry_mut, etc.).

While here, I've also simplified handling and aggregating of rights on
the Entry object. Instead of storing base and inheriting rights as
separate fields, they are now aggregated into one struct EntryRights
which features convenient constructors for each possible combination; i.e.,
when only base rights are set, or both base and inheriting are set, or
both are left as empty. Since we do need to be able to mutate those
rights down the line in fdstat_set_rights syscall, this object
is kept behind a RefCell (note no Rc since we don't need to pass it
around anywhere).

The descriptor field in Entry is now kept behind Rc<RefCell<..>> combo
since we not only need to mutate it down the line, but we also need to
be able to pass it around (as part of the machinery making poll_oneoff
work).

I've also removed as_file and try_clone methods on Descriptor struct
since they were adding more noise than necessary, and making them work
with Rc was unnecessarily complicated.

Finally, I've converted the get_dir_from_os_handle function into a
method attached to the OsHandle itself, called dir_stream. IMHO,
it makes more sense to have it there directly as a method than as a separate
function.

Wasmtime GitHub notifications bot (Mar 27 2020 at 08:34):

kubkon merged PR #1412.

Last updated: Dec 23 2024 at 12:05 UTC