[wip] rust runtime channel ownership #648
Conversation
| @@ -85,7 +85,7 @@ struct WasmInterface { | |||
| impl WasmInterface { | |||
| /// Generate a randomized handle. Handles are random to prevent accidental dependency on | |||
| /// particular values or sequence. See https://github.com/project-oak/oak/pull/347 | |||
| fn allocate_new_handle(&mut self, channel: ChannelEither) -> AbiHandle { | |||
| fn allocate_new_handle(&mut self, channel: ChannelEitherOwned) -> AbiHandle { | |||
There was a problem hiding this comment.
Would it make sense to introduce this "randomized" handle concept to the ABI interface? I don't think it's specific to Wasm.
| use crate::node::{load_wasm, NodeConfiguration}; | ||
| use crate::platform; | ||
| use crate::proto; | ||
| use crate::proto::application::NodeConfiguration_oneof_config_type; | ||
|
|
||
| /// Runtime structure for configuring and running a set of Oak nodes. | ||
| #[derive(PartialEq, Eq, Hash)] |
There was a problem hiding this comment.
How about the following (trying to simplify things a bit):
struct NodeId(usize);
struct Node {
handles: HashSet<Handle>;
/* ... */
};
struct ChannelId(usize);
struct Channel {
/* ... */
}
/// All handles have the same representation.
/// Ideally this would also be already randomized (as per my other comment).
struct Handle(usize);
// Possibly come up with a better name...
enum Object {
ChannelWriter { channel_id: ChannelId },
ChannelReader { channel_id: ChannelId },
}
struct Runtime {
nodes: HashMap<NodeId, Node>,
channels: HashMap<ChannelId, Channel>,
objects: HashMap<Handle, Object>,
}Alternatively, we could use Weak as in
enum Object {
ChannelWriter { channel: Weak<Channel> }
ChannelReader{ channel: Weak<Channel> }
}and avoid the intermediate ChannelId (basically use the memory address as the id), but I feel that having explicit, printable ids for everything (including nodes and channels) would be useful anyways.
Thoughts?
Also would like to hear @daviddrysdale 's opinion.
There was a problem hiding this comment.
FTR
I realised that effectively there are already cases in which just having a node talk to its direct channel(reader)s is not sufficient. For instance:
in which in order to wait on channels we need to go back to the Runtime.
That line may as well read
runtime.wait_on_channels([handle])and shows how the API I am suggesting may look like if we extended to other channel operations, e.g.
runtime.read(handle)
| channels: platform::Mutex<Vec<ChannelInfo>>, | ||
| channel_readers: platform::Mutex<HashMap<ChannelReader, ChannelRef>>, | ||
| channel_writers: platform::Mutex<HashMap<ChannelWriter, ChannelRef>>, | ||
| next_reader: AtomicUsize, | ||
| next_writer: AtomicUsize, |
There was a problem hiding this comment.
You might want to consider moving this into a separate Channels struct that just handles operations on channels. Then you would probably want to do something similar for Node related data once you start refactoring that part. This allows you to split the runtime into different parts that you compose in the main Runtime struct. That of course assumes that most operations on channels are unrelated to the node setup, but I think that is the case?
|
Yes, I'll make this more presentable and incorporate some of the ideas we've had |
|
Closing in favour of #680 |
Very rough draft, rust_runtime compiles. Code isn't supposed to be good yet, just an idea of whats structs/how ownership might work
Checklist
cover any TODOs and/or unfinished work.
construction.