Skip to content

Host rust#2500

Merged
sporksmith merged 5 commits intoshadow:mainfrom
sporksmith:host-rust
Nov 2, 2022
Merged

Host rust#2500
sporksmith merged 5 commits intoshadow:mainfrom
sporksmith:host-rust

Conversation

@sporksmith
Copy link
Copy Markdown
Contributor

@sporksmith sporksmith commented Oct 29, 2022

  • The C implementation of Host is renamed to HostCInternal and its
    methods host_* renamed to hostc_*. This type and these methods are
    now meant only to be used from the Rust Host.
  • The Rust Host is now the canonical Host type. It is currently not much
    more than a wrapper around a HostCInternal, with the host_* methods
    implemented by calling their original hostc_* implementations, but we
    should now be able to start moving fields from HostCInternal to Host,
    and start replacing the thin proxy methods with Rust native implementations.
  • The Host now has a Root object that we can use for cheap interior
    mutability and reference counting of objects associated with that Host.
  • Host is no longer ref-counted. There is no longer a host_ref nor
    hostc_ref. This is because Host is !Sync, so could not be stored
    inside a ref-counted wrapper without re-adding a Host lock wrapper
    to make it Sync. e.g. it would need to be something like an
    Arc<AtomicRefCell<Host>>. While the extra locking overhead in the scheduler
    probably wouldn't be that noticeable, it'd make it difficult to make access to the
    &Host or its internal &Root cheaply globally accessible.
  • A reference to the current Host can now be retrived from the Worker via
    Worker::with_active_host. This lets us get to the Host from places
    that previously fetched it from a stored pointer.
  • Host methods now all take &Host; there are no &mut Host methods.
    With so much inside the Host, and since we want to be able to access it via
    a global (at least for now), anything holding a &mut Host would prevent
    anything else being able to retrieve any reference to the Host.
  • We do the same with the Worker, for analagous reasons.
  • The scheduler now moves the Host value instead of passing a &Host.
    This allows the Host to be stored in the Worker's thread-local,
    and borrowed as-needed via Worker::with_active_host.

In a separate commit, the Host's EventQueue is moved from HostCInternal to Host, along with some corresponding method implementations. This doesn't strictly need to be part of this PR, but is a proof of concept of what the next stages of this migration will look like.

@sporksmith sporksmith self-assigned this Oct 29, 2022
@github-actions github-actions bot added Component: Libraries Support functions like LD_PRELOAD and logging Component: Main Composing the core Shadow executable labels Oct 29, 2022
@codecov
Copy link
Copy Markdown

codecov bot commented Oct 29, 2022

Codecov Report

Base: 67.72% // Head: 66.81% // Decreases project coverage by -0.91% ⚠️

Coverage data is based on head (713627b) compared to base (c0701db).
Patch coverage: 83.01% of modified lines in pull request are covered.

❗ Current head 713627b differs from pull request most recent head 9187d47. Consider uploading reports for the commit 9187d47 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2500      +/-   ##
==========================================
- Coverage   67.72%   66.81%   -0.92%     
==========================================
  Files         191      190       -1     
  Lines       27768    27944     +176     
  Branches     5542     5533       -9     
==========================================
- Hits        18807    18671     -136     
- Misses       4539     4905     +366     
+ Partials     4422     4368      -54     
Flag Coverage Δ
tests 66.81% <83.01%> (-0.92%) ⬇️

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

Impacted Files Coverage Δ
...ib/shadow-shim-helper-rs/src/rootedcell/refcell.rs 89.28% <ø> (ø)
src/main/core/scheduler/thread_per_host.rs 0.00% <0.00%> (ø)
src/main/host/syscall/handler/unistd.rs 53.64% <20.00%> (-16.82%) ⬇️
src/main/core/logger/shadow_logger.rs 67.25% <50.00%> (ø)
src/main/core/work/event.rs 49.45% <50.00%> (-8.80%) ⬇️
src/main/host/context.rs 61.29% <60.00%> (-2.35%) ⬇️
src/main/core/scheduler/mod.rs 62.96% <66.66%> (ø)
src/main/utility/mod.rs 51.16% <66.66%> (ø)
src/main/host/host.rs 84.49% <84.01%> (+2.01%) ⬆️
src/main/core/worker.rs 74.69% <89.18%> (+0.24%) ⬆️
... and 44 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@sporksmith sporksmith force-pushed the host-rust branch 2 times, most recently from b9aee96 to 45c24dc Compare October 31, 2022 14:55
@sporksmith sporksmith marked this pull request as ready for review October 31, 2022 14:57
@sporksmith
Copy link
Copy Markdown
Contributor Author

I think this should be performance neutral, but kicked off a benchmark to be sure https://github.com/shadow/benchmark/actions/runs/3362534049

@sporksmith
Copy link
Copy Markdown
Contributor Author

I think this should be performance neutral, but kicked off a benchmark to be sure https://github.com/shadow/benchmark/actions/runs/3362534049

It looks like there may be a slight performance regression: https://github.com/shadow/benchmark-results/blob/master/tor/2022-10-31-T15-28-48/plots/run_time.png

Looking into it; probably one of the first things I'll try is boxing the Host as @stevenengler suggested, so that moving it around is a bit cheaper

@sporksmith
Copy link
Copy Markdown
Contributor Author

Locally on the tor-minimal test it looks like boxing the Host went from making this PR a slight perf loss to a slight perf gain, but it's all close to being in the noise.

New benchmark running with the boxed host: https://github.com/shadow/benchmark/actions/runs/3371218645

* The C implementation of `Host` is renamed to `HostCInternal` and its
  methods `host_*` renamed to `hostc_*`. This type and these methods are
  now meant only to be used from the Rust `Host`.
* The Rust `Host` is now the canonical `Host` type. It is currently not much
  more than a wrapper around a `HostCInternal`, with the `host_*` methods
  implemented by calling their original `hostc_*` implementations, but we
  should now be able to start moving fields from `HostCInternal` to `Host`,
  and start replacing the thin proxy methods with Rust native implementations.
* The `Host` now has a `Root` object that we can use for cheap interior
  mutability and reference counting of objects associated with that `Host`.
* `Host` is no longer ref-counted. There is no longer a `host_ref` nor
  `hostc_ref`. This is because `Host` is `!Sync`, so could not be stored
  inside a ref-counted wrapper without re-adding a `Host` lock wrapper
  to make it `Sync`. e.g. it would need to be something like an
  `Arc<AtomicRefCell<Host>>`. While the extra locking overhead in the scheduler
  probably wouldn't be that noticeable, make it difficult to make access to the
  `&Host` or its internal `&Root` cheaply globally accessible.
* A reference to the current `Host` can now be retrived from the `Worker` via
  `Worker::with_active_host`. This lets us get to the `Host` from places
  that previously fetched it from a stored pointer.
* `Host` methods now all take `&Host`; there are no `&mut Host` methods.
  With so much inside the `Host`, and since we want to be able to access it via
  a global (at least for now), anything holding a `&mut Host` would prevent
  anything else being able to retrieve any reference to the `Host`.
* We do the same with the `Worker`, for analagous reasons.
* The scheduler now moves the `Host` value instead of passing a `&Host`.
  This allows the `Host` to be stored in the `Worker`'s thread-local,
  and borrowed as-needed via `Worker::with_active_host`.
It appears that moving the unboxed Host around had a significant
performance cost. Locally running
`./setup build --werror --test --extra && hyperfine './setup test --extra tor-minimal'`

origin/main (baseline):

```
Benchmark 1: ./setup test --extra tor-minimal
  Time (mean ± σ):     16.000 s ±  0.351 s    [User: 17.036 s, System: 5.998 s]
  Range (min … max):   15.568 s … 16.505 s    10 runs
```

This PR, before boxing:

```
Benchmark 1: ./setup test --extra tor-minimal
  Time (mean ± σ):     16.232 s ±  0.513 s    [User: 17.389 s, System: 6.117 s]
  Range (min … max):   15.520 s … 16.998 s    10 runs
```

This commit (i.e. after boxing):

```
Benchmark 1: ./setup test --extra tor-minimal
  Time (mean ± σ):     15.622 s ±  0.491 s    [User: 16.695 s, System: 5.920 s]
  Range (min … max):   15.106 s … 16.570 s    10 runs
```
This simplifies the responsibilities of the caller, which previously was
responsible for setting each Host as active and ultimately returning
each Host to the iterator.
@sporksmith
Copy link
Copy Markdown
Contributor Author

After boxing the Host it looks about the same as the last nightly: https://github.com/shadow/benchmark-results/blob/master/tor/2022-11-01-T16-50-29/plots/run_time.png

@sporksmith sporksmith enabled auto-merge November 2, 2022 00:38
This allows us to remove the previously unsound api Host::random, which
returned a mutable reference from an immutable reference without using
interior mutability.

Rather than trying to expose the Rng directly to C, with the needed
function object, we implement the rng APIs that the C code needs
directly on the Host.

After this change we no longer need the Random C APIs, so those are
removed.
@sporksmith sporksmith merged commit 3426db6 into shadow:main Nov 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Component: Libraries Support functions like LD_PRELOAD and logging Component: Main Composing the core Shadow executable

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants