Skip to content

Rooted*: make public APIs inline#2750

Merged
sporksmith merged 1 commit intoshadow:mainfrom
sporksmith:rooted-inline
Feb 15, 2023
Merged

Rooted*: make public APIs inline#2750
sporksmith merged 1 commit intoshadow:mainfrom
sporksmith:rooted-inline

Conversation

@sporksmith
Copy link
Copy Markdown
Contributor

@sporksmith sporksmith commented Feb 14, 2023

By default, Rust doesn't inline calls across crate boundaries. Since these functions are small, called frequently, and from across a crate boundary, we probably want them to be inline. We force this by marking them #[inline], which is also done in the analogous functions in std.

A quick local benchmark using the tor minimal test shows there might be a measurable improvement here, though it's inconclusive since the confidence intervals overlap substantially:

Before:

Benchmark 1: ./setup test --extra tor-minimal
  Time (mean ± σ):     17.424 s ±  0.455 s    [User: 18.446 s, System: 6.024 s]
  Range (min … max):   16.818 s … 18.123 s    10 runs

After:

Benchmark 1: ./setup test --extra tor-minimal
  Time (mean ± σ):     17.119 s ±  0.482 s    [User: 18.211 s, System: 5.993 s]
  Range (min … max):   16.534 s … 18.020 s    10 runs

@github-actions github-actions bot added the Component: Libraries Support functions like LD_PRELOAD and logging label Feb 14, 2023
@sporksmith
Copy link
Copy Markdown
Contributor Author

Started a larger tor benchmark, mostly out of curiosity. I'm fairly confident this should be performance-neutral at worst. https://github.com/shadow/benchmark/actions/runs/4176543974

@sporksmith sporksmith marked this pull request as ready for review February 14, 2023 17:46
By default, Rust doesn't inline calls across crate boundaries. Since
these functions are called frequently, and from across a crate boundary,
we probably want them to be inline. We force this by marking them
`#[inline]`, which is also done in the The analagous functions in `std`.

A quick local benchmark using the tor minimal test shows there might be
a measurable improvement here, though it's inconclusive since the
confidence intervals overlap substantially:

Before:

```
Benchmark 1: ./setup test --extra tor-minimal
  Time (mean ± σ):     17.424 s ±  0.455 s    [User: 18.446 s, System: 6.024 s]
  Range (min … max):   16.818 s … 18.123 s    10 runs
```

After:

```
Benchmark 1: ./setup test --extra tor-minimal
  Time (mean ± σ):     17.119 s ±  0.482 s    [User: 18.211 s, System: 5.993 s]
  Range (min … max):   16.534 s … 18.020 s    10 runs
```
@sporksmith
Copy link
Copy Markdown
Contributor Author

The benchmark runner's tor benchmark looks neutral. Oh well, merging anyway, if nothing else so it doesn't come up again :)

@sporksmith sporksmith enabled auto-merge (squash) February 15, 2023 16:08
@sporksmith sporksmith merged commit 11bd2f7 into shadow:main Feb 15, 2023
@sporksmith sporksmith deleted the rooted-inline branch February 15, 2023 16:21
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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants