Skip to content

Pass references explicitly instead of getting from global Worker#1386

Merged
sporksmith merged 1 commit intoshadow:devfrom
sporksmith:active-objs
Jun 2, 2021
Merged

Pass references explicitly instead of getting from global Worker#1386
sporksmith merged 1 commit intoshadow:devfrom
sporksmith:active-objs

Conversation

@sporksmith
Copy link
Copy Markdown
Contributor

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.

@sporksmith sporksmith requested a review from stevenengler May 28, 2021 23:55
@codecov
Copy link
Copy Markdown

codecov bot commented May 28, 2021

Codecov Report

Merging #1386 (07d8ded) into dev (82637cc) will decrease coverage by 0.04%.
The diff coverage is 48.57%.

❗ Current head 07d8ded differs from pull request most recent head 3d1cef0. Consider uploading reports for the commit 3d1cef0 to get more accurate results
Impacted file tree graph

@@            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     
Flag Coverage Δ
tests 54.34% <48.57%> (-0.05%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/main/host/thread_preload.c 0.00% <0.00%> (ø)
src/main/host/process.c 68.01% <57.14%> (-0.16%) ⬇️
src/main/host/thread_ptrace.c 49.50% <60.00%> (ø)
src/main/core/worker.c 74.00% <75.00%> (-2.34%) ⬇️
src/main/host/syscall/mman.c 61.36% <100.00%> (ø)
src/main/host/thread.c 64.81% <100.00%> (+0.66%) ⬆️
src/support/logger/rust_bindings/src/lib.rs 36.59% <0.00%> (+0.31%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 82637cc...3d1cef0. Read the comment docs.

@sporksmith sporksmith self-assigned this May 28, 2021
@sporksmith sporksmith added Component: Main Composing the core Shadow executable Tag: Performance Related to improving shadow's run-time Type: Maintenance Refactoring, cleanup, documenation, or process improvements labels May 28, 2021
sporksmith added a commit to sporksmith/shadow that referenced this pull request Jun 1, 2021
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.
Copy link
Copy Markdown
Contributor

@stevenengler stevenengler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@sporksmith sporksmith enabled auto-merge June 2, 2021 15:08
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.
@sporksmith sporksmith merged commit ec92fb5 into shadow:dev Jun 2, 2021
sporksmith added a commit to sporksmith/shadow that referenced this pull request Jun 2, 2021
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.
sporksmith added a commit that referenced this pull request Jun 2, 2021
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Component: Main Composing the core Shadow executable Tag: Performance Related to improving shadow's run-time Type: Maintenance Refactoring, cleanup, documenation, or process improvements

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants