Conversation
Codecov ReportBase: 67.72% // Head: 66.81% // Decreases project coverage by
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
Flags with carried forward coverage won't be shown. Click here to find out 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. |
b9aee96 to
45c24dc
Compare
|
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 |
|
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.
|
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 |
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.
Hostis renamed toHostCInternaland itsmethods
host_*renamed tohostc_*. This type and these methods arenow meant only to be used from the Rust
Host.Hostis now the canonicalHosttype. It is currently not muchmore than a wrapper around a
HostCInternal, with thehost_*methodsimplemented by calling their original
hostc_*implementations, but weshould now be able to start moving fields from
HostCInternaltoHost,and start replacing the thin proxy methods with Rust native implementations.
Hostnow has aRootobject that we can use for cheap interiormutability and reference counting of objects associated with that
Host.Hostis no longer ref-counted. There is no longer ahost_refnorhostc_ref. This is becauseHostis!Sync, so could not be storedinside a ref-counted wrapper without re-adding a
Hostlock wrapperto make it
Sync. e.g. it would need to be something like anArc<AtomicRefCell<Host>>. While the extra locking overhead in the schedulerprobably wouldn't be that noticeable, it'd make it difficult to make access to the
&Hostor its internal&Rootcheaply globally accessible.Hostcan now be retrived from theWorkerviaWorker::with_active_host. This lets us get to theHostfrom placesthat previously fetched it from a stored pointer.
Hostmethods now all take&Host; there are no&mut Hostmethods.With so much inside the
Host, and since we want to be able to access it viaa global (at least for now), anything holding a
&mut Hostwould preventanything else being able to retrieve any reference to the
Host.Worker, for analagous reasons.Hostvalue instead of passing a&Host.This allows the
Hostto be stored in theWorker's thread-local,and borrowed as-needed via
Worker::with_active_host.In a separate commit, the Host's
EventQueueis moved fromHostCInternaltoHost, 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.