Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Expose runtime structure #148

Merged
merged 2 commits into from
Oct 26, 2022
Merged

Conversation

ollie-etl
Copy link
Contributor

@ollie-etl ollie-etl commented Oct 25, 2022

A recreation of #88, which doesn't seem to be updated. See discussion in that PR. Closes #91.

The motivationis as expressed in #91 and #88, to allow access to the runtime. An example of where this is used is criterion. If this is merged and a release issued, I will put a PR in to criterion with AsyncExecutor trait impl as a criterion feature. Until then, the following works

struct AsyncRuntime(tokio_uring::Runtime);

impl criterion::async_executor::AsyncExecutor for &AsyncRuntime {
    fn block_on<T>(&self, future: impl futures::Future<Output = T>) -> T {
        self.0.block_on(future)
    }
}

// ...

fn bench(c: &mut Criterion) {
    // ...
    let mut ring_opts = tokio_uring::uring_builder();  
    let mut builder = tokio_uring::builder();
    builder.uring_builder(&ring_opts);
    let runtime = AsyncRuntime(tokio_uring::Runtime::new(&builder).unwrap());
    let runtime = &runtime;

   c.bench_with_input(BenchmarkId::new("input_example", size), &size, |b, &s| {
        // Insert a call to `to_async` to convert the bencher to async mode.
        // The timing loops are the same as with the normal bencher.
        b.to_async(runtime).iter(|| do_something(s));
    });
}

@ollie-etl
Copy link
Contributor Author

@FrankReh Is that a suitable usage statement?

@FrankReh
Copy link
Collaborator

@ollie-etl Thanks for the quick uptake. I have more questions about the commit message than I do the files changes. The changed files look good to me.

Is the usage showing an example bench that would act like a main for criterion benchmarking or does all that setup need to be duplicated for each benchmark or each file that would have benchmarking in it?

@FrankReh
Copy link
Collaborator

@Noah-Kennedy If these changes look good to you too, will you merge? My question about the running example can wait.

@ollie-etl
Copy link
Contributor Author

@FrankReh With criterion, you can share setup per group, https://bheisler.github.io/criterion.rs/book/user_guide/benchmarking_with_inputs.html#benchmarking-with-a-range-of-values.

My example is only a single function.

The newtyped trait implementation is something that I'd hope to get up-streamed to criterion. That will require a new published version

@FrankReh
Copy link
Collaborator

Just going to wait for @Noah-Kennedy to accept this one. It looks good to me. I wish there were an example binary using criterion but we have lots of other functionality that doesn't come with an example binary either so there's no need to start that here.

Copy link
Collaborator

@FrankReh FrankReh left a comment

Choose a reason for hiding this comment

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

These changes look reasonable and are what folks have been asking for to be able to use criterion.

@Noah-Kennedy Noah-Kennedy merged commit cb189fa into tokio-rs:master Oct 26, 2022
@Noah-Kennedy
Copy link
Contributor

It looks like the benchmarks spend most of their time in FuturesUnordered. I'm going to try and cleanup our results a bit and see what can be optimized.

flamegraph

@Noah-Kennedy
Copy link
Contributor

We should probably move this over to discord or to a dedicated issue thread.

@Noah-Kennedy
Copy link
Contributor

Discord seems better.

@Noah-Kennedy
Copy link
Contributor

Ah, meant to post this svg in another thread.

ollie-etl pushed a commit to etlsystems/tokio-uring that referenced this pull request Oct 27, 2022
- tokio-rs#148 exposed Runtime. It is now possible to use Criterions
async support directly. Do so

- Adds criterion support for flamegraph generation of only the
  benchmarked code (pprof)

- Now crashes on second call to block_on, as the AsyncFd still exists
  Introduced in tokio-rs#142
Noah-Kennedy pushed a commit that referenced this pull request Nov 5, 2022
# 0.4.0 (November 5th, 2022)

### Fixed

- Fix panic in Deref/DerefMut for Slice extending into uninitialized
part of the buffer ([#52])
- docs: all-features = true ([#84])
- fix fs unit tests to avoid parallelism ([#121])
- Box the socket address to allow moving the Connect future ([#126])
- rt: Fix data race ([#146])

### Added

- Implement fs::File::readv_at()/writev_at() ([#87])
- fs: implement FromRawFd for File ([#89])
- Implement `AsRawFd` for `TcpStream` ([#94])
- net: add TcpListener.local_addr method ([#107])
- net: add TcpStream.write_all ([#111])
- driver: add Builder API as an option to start ([#113])
- Socket and TcpStream shutdown ([#124])
- fs: implement fs::File::from_std ([#131])
- net: implement FromRawFd for TcpStream ([#132])
- fs: implement OpenOptionsExt for OpenOptions ([#133])
- Add NoOp support ([#134])
- Add writev to TcpStream ([#136])
- sync TcpStream, UnixStream and UdpSocket functionality ([#141])
- Add benchmarks for no-op submission ([#144])
- Expose runtime structure ([#148])

### Changed

- driver: batch submit requests and add benchmark ([#78])
- Depend on io-uring version ^0.5.8 ([#153])

### Internal Improvements

- chore: fix clippy lints ([#99])
- io: refactor post-op logic in ops into Completable ([#116])
- Support multi completion events: v2 ([#130])
- simplify driver operation futures ([#139])
- rt: refactor runtime to avoid Rc\<RefCell\<...>> ([#142])
- Remove unused dev-dependencies ([#143])
- chore: types and fields explicitly named ([#149])
- Ignore errors from uring while cleaning up ([#154])
- rt: drop runtime before driver during shutdown ([#155])
- rt: refactor drop logic ([#157])
- rt: fix error when calling block_on twice ([#162])

### CI changes

- chore: update actions/checkout action to v3 ([#90])
- chore: add all-systems-go ci check ([#98])
- chore: add clippy to ci ([#100])
- ci: run cargo test --doc ([#135])


[#52]: #52
[#78]: #78
[#84]: #84
[#87]: #87
[#89]: #89
[#90]: #90
[#94]: #94
[#98]: #98
[#99]: #99
[#100]: #100
[#107]: #107
[#111]: #111
[#113]: #113
[#116]: #116
[#121]: #121
[#124]: #124
[#126]: #126
[#130]: #130
[#131]: #131
[#132]: #132
[#133]: #133
[#134]: #134
[#135]: #135
[#136]: #136
[#139]: #139
[#141]: #141
[#142]: #142
[#143]: #143
[#144]: #144
[#146]: #146
[#148]: #148
[#149]: #149
[#153]: #153
[#154]: #154
[#155]: #155
[#157]: #157
[#162]: #162
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow access for Runtime struct?
3 participants