Pass references explicitly instead of getting from global Worker#1386
Pass references explicitly instead of getting from global Worker#1386sporksmith merged 1 commit intoshadow:devfrom
Conversation
Codecov Report
@@ Coverage Diff @@
## dev #1386 +/- ##
==========================================
- Coverage 54.38% 54.34% -0.05%
==========================================
Files 136 136
Lines 20499 20465 -34
Branches 5170 5169 -1
==========================================
- Hits 11149 11121 -28
+ Misses 6426 6416 -10
- Partials 2924 2928 +4
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
This is conceptually a follow-up to shadow#1386, but doesn't actually depend on it. Once both this and shadow#1386 are in, we can remove getActiveHost, getActiveProcess, and getActiveThread entirely. This is helpful for porting to Rust since we don't have or want those to be globals there. In C code it suffices to pass around the "deepest" current context object, since they all have "up" pointers. e.g. given a Thread we can get the corresponding Process and Host. We might eventually want to replace these parameters with their Context counterparts in shadow#1386, but this seems like a useful intermediate step.
stevenengler
left a comment
There was a problem hiding this comment.
I'm having trouble following the design, I think there are too many layers of indirection to fit in my brain at once, but I know this is caused by integrating C with Rust and should become clearer as you continue moving things to Rust. So I'm good with just going with your judgement, and once it gets further along I'll look over all of the code to get a big-picture view again.
Adds a `context` module, which provides several *Context* structs,
intended to bundle together current relevant objects in the hierarchy.
These are meant to replace `worker_getActiveThread`, etc. Passing around
the current context explicitly instead of putting them in globals both
allows us to avoid interior mutability (and its associated runtime cost
and fallibility), and lets us keep a hierarchical object structure (e.g.
allow holding a mutable Process and a mutable Thread belonging to that
process simultaneously).
Most code (e.g. syscall handlers) can take a `ThreadContext` argument
and use that to manipulate anything on the Host. The *current* `Thread`
and `Process` should typically be accessed directly. e.g. since a
mutable reference to the current `Thread` exists at
`ThreadContext::thread`, it *cannot* also be accessible via
`ThreadContext::process` or `ThreadContext::host`.
The manner in which they're unavailable isn't implemented yet, but the
current plan is that they'll be temporarily removed from their
collections. e.g. something conceptually like:
```
impl Process {
pub fn continue_thread(&mut self, host_ctx: &mut HostContext, tid: ThreadId) {
let thread = self.threads.get_mut(tid).take();
thread.continue(&mut host_ctx.add_process(self));
self.threads.get_mut(tid).replace(thread);
}
}
```
The Context objects are designed to allow simultaneously borrowing from
multiple of their objects. This is currently implemented by exposing
their fields directly - Rust then allows each field to be borrowed
independently. This could alternatively be implemented by providing
methods that borrow some or all of their internal references
simultaneously.
Also:
* Move implementations of setActive* and getActive* to Rust Worker. The
Rust Worker *doesn't* expose references to the active objects themselves
via the Rust API; only to some immutable info about those objects.
* The ShadowLogger now clones an Arc with the relevant Host info,
instead of copying the string name itself and rendering the IP address
from the calling thread. I wasn't sure whether this would actually be a
win or not, but experimentally `make test -j12` is ~2% faster cloning
the Arc than copying the data in both debug and release builds.
This is conceptually a follow-up to shadow#1386, but doesn't actually depend on it. Once both this and shadow#1386 are in, we can remove getActiveHost, getActiveProcess, and getActiveThread entirely. This is helpful for porting to Rust since we don't have or want those to be globals there. In C code it suffices to pass around the "deepest" current context object, since they all have "up" pointers. e.g. given a Thread we can get the corresponding Process and Host. We might eventually want to replace these parameters with their Context counterparts in shadow#1386, but this seems like a useful intermediate step.
Remove C usage of getActiveHost, getActiveProcess, getActiveThread This is conceptually a follow-up to #1386, but doesn't actually depend on it. Once both this and #1386 are in, we can remove getActiveHost, getActiveProcess, and getActiveThread entirely. This is helpful for porting to Rust since we don't have or want those to be globals there. In C code it suffices to pass around the "deepest" current context object, since they all have "up" pointers. e.g. given a Thread we can get the corresponding Process and Host. We might eventually want to replace these parameters with their Context counterparts in #1386, but this seems like a useful intermediate step.
Adds a
contextmodule, which provides several Context structs,intended to bundle together current relevant objects in the hierarchy.
These are meant to replace
worker_getActiveThread, etc. Passing aroundthe current context explicitly instead of putting them in globals both
allows us to avoid interior mutability (and its associated runtime cost
and fallibility), and lets us keep a hierarchical object structure (e.g.
allow holding a mutable Process and a mutable Thread belonging to that
process simultaneously).
Most code (e.g. syscall handlers) can take a
ThreadContextargumentand use that to manipulate anything on the Host. The current
Threadand
Processshould typically be accessed directly. e.g. since amutable reference to the current
Threadexists atThreadContext::thread, it cannot also be accessible viaThreadContext::processorThreadContext::host.The manner in which they're unavailable isn't implemented yet, but the
current plan is that they'll be temporarily removed from their
collections. e.g. something conceptually like:
The Context objects are designed to allow simultaneously borrowing from
multiple of their objects. This is currently implemented by exposing
their fields directly - Rust then allows each field to be borrowed
independently. This could alternatively be implemented by providing
methods that borrow some or all of their internal references
simultaneously.
Also:
Move implementations of setActive* and getActive* to Rust Worker. The
Rust Worker doesn't expose references to the active objects themselves
via the Rust API; only to some immutable info about those objects.
The ShadowLogger now clones an Arc with the relevant Host info,
instead of copying the string name itself and rendering the IP address
from the calling thread. I wasn't sure whether this would actually be a
win or not, but experimentally
make test -j12is ~2% faster cloningthe Arc than copying the data in both debug and release builds.