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.

Sign up for GitHub

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

Jump to bottom

io: reduce syscalls in poll_read #4840

Merged
merged 1 commit into from
Jul 26, 2022
Merged

io: reduce syscalls in poll_read #4840

merged 1 commit into from
Jul 26, 2022

Conversation

Noah-Kennedy
Copy link
Contributor

@Noah-Kennedy Noah-Kennedy commented Jul 15, 2022 •
edited
Loading

As the epoll documentation points out, a read that only partially fills a buffer is sufficient to show that the socket buffer has been drained.

We can take advantage of this by clearing readiness in this case, which seems to significantly improve performance under certain conditions.

@Darksonn Darksonn added A-tokio Area: The main tokio crate M-io Module: tokio/io labels Jul 15, 2022
@Darksonn Darksonn requested a review from Thomasdezeeuw July 15, 2022 20:56
@Darksonn
Copy link
Contributor

Does this mean that we will advance the ReadBuf while also returning WouldBlock?

@Noah-Kennedy
Copy link
Contributor Author

Noah-Kennedy commented Jul 15, 2022 •
edited
Loading

@Darksonn yeah, the advancement amount is now set inside the closure independent of the return value of the closure.

@Darksonn
Copy link
Contributor

Actually, never mind. It appears that even if the readiness is cleared, this still returns Ready(n). This will cause the caller to attempt another poll_read, but since we keep track of readiness, this does not actually result in another syscall.

@Noah-Kennedy
Copy link
Contributor Author

So what you are saying is that there is still room for further perf gains? Most excellent!

@Noah-Kennedy
Copy link
Contributor Author

Side note: theoretically this should also be applicable to writes as well.

@Darksonn
Copy link
Contributor

Darksonn commented Jul 15, 2022 •
edited
Loading

No, I'm saying that my concern didn't apply. Returning Poll::Pending and advancing the ReadBuf is against the contract of the AsyncRead trait.

@Noah-Kennedy
Copy link
Contributor Author

Ah, makes more sense!

@Noah-Kennedy
Copy link
Contributor Author

Was slightly confused

@carllerche
Copy link
Member

The way I implemented it originally (advance buffer but return pending) was mostly a hack to test out the concept. We probably don't want to use this strategy in practice. We probably just want to avoid using the poll_io helper here and use one that can return Ready while unsetting readiness.

We also only want to use this optimization on platforms for which we know it holds. This includes Linux at the very least but also probably kqueue. It definitely doesn't include windows as of now.

Thomasdezeeuw
Thomasdezeeuw reviewed Jul 16, 2022
View reviewed changes
Copy link
Contributor

@Thomasdezeeuw Thomasdezeeuw left a comment

Choose a reason for hiding this comment

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

This strategy work on Unix (epoll and kqueue), but doesn't work on Windows as Mio uses the WouldBlock error to re-register I/O interest internally, see https://github.com/tokio-rs/mio/blob/add75b1dfff13d2b81ca40b23708996296fc4c44/src/sys/windows/mod.rs#L86-L96.

tokio/src/io/poll_evented.rs Outdated Show resolved Hide resolved
@Noah-Kennedy Noah-Kennedy force-pushed the reduce-syscalls branch 5 times, most recently from acca220 to b3d8fad Compare July 26, 2022 00:29
@carllerche @Noah-Kennedy
io: reduce syscalls in poll_read
f61d606 
As the [epoll documentation points out](https://man7.org/linux/man-pages/man7/epoll.7.html), a read that only partially fills a buffer is sufficient to show that the socket buffer has been drained.

We can take advantage of this by clearing readiness in this case, which seems to significantly improve performance under certain conditions.
@Noah-Kennedy Noah-Kennedy marked this pull request as ready for review July 26, 2022 02:08
@Noah-Kennedy Noah-Kennedy enabled auto-merge (squash) July 26, 2022 02:10
Thomasdezeeuw
Thomasdezeeuw approved these changes Jul 26, 2022
View reviewed changes
Copy link
Contributor

@Thomasdezeeuw Thomasdezeeuw left a comment

Choose a reason for hiding this comment

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

From Mio's side this looks good. However note that technically Mio doesn't guarantee that reading less bytes means readiness is drained, but all Unix-lie OSs we support do, so Mio guarantee is technically speaking violated here. That said, I've been in favour of switching to the Unix behaviour for Unix as well (since Mio controls it directly).

@Noah-Kennedy Noah-Kennedy merged commit 28ce4ee into master Jul 26, 2022
@Noah-Kennedy Noah-Kennedy deleted the reduce-syscalls branch July 26, 2022 05:52
Darksonn added a commit that referenced this pull request Jul 26, 2022
@Darksonn
Also update file changed in #4840
fe8aefa
@Darksonn Darksonn mentioned this pull request Sep 1, 2022
Merged
Noah-Kennedy added a commit that referenced this pull request Sep 2, 2022
@Noah-Kennedy
io: reduce syscalls in poll_write
bbb96fc 
This applies the same optimization made in #4840 to writes.
@Noah-Kennedy Noah-Kennedy mentioned this pull request Sep 2, 2022
Merged
Noah-Kennedy added a commit that referenced this pull request Sep 2, 2022
@Noah-Kennedy
io: reduce syscalls in poll_write
d5f1242 
This applies the same optimization made in #4840 to writes.
Noah-Kennedy added a commit that referenced this pull request Sep 2, 2022
@Noah-Kennedy
io: reduce syscalls in poll_write
8d517d3 
This applies the same optimization made in #4840 to writes.
Noah-Kennedy added a commit that referenced this pull request Sep 2, 2022
@Noah-Kennedy
io: reduce syscalls in poll_write (#4970)
c2612b4 
This applies the same optimization made in #4840 to writes.
crapStone pushed a commit to Calciumdibromid/CaBr2 that referenced this pull request Sep 11, 2022
Update Rust crate tokio to 1.21.0 (#1532)
62b6b26 
This PR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
| [tokio](https://tokio.rs) ([source](https://github.com/tokio-rs/tokio)) | dependencies | minor | `1.20.1` -> `1.21.0` |
| [tokio](https://tokio.rs) ([source](https://github.com/tokio-rs/tokio)) | dev-dependencies | minor | `1.20.1` -> `1.21.0` |

---

### Release Notes

<details>
<summary>tokio-rs/tokio</summary>

### [`v1.21.0`](https://github.com/tokio-rs/tokio/releases/tag/tokio-1.21.0)

[Compare Source](tokio-rs/tokio@tokio-1.20.1...tokio-1.21.0)

##### 1.21.0 (September 2, 2022)

This release is the first release of Tokio to intentionally support WASM. The `sync,macros,io-util,rt,time` features are stabilized on WASM. Additionally the wasm32-wasi target is given unstable support for the `net` feature.

##### Added

-   net: add `device` and `bind_device` methods to TCP/UDP sockets ([#&#8203;4882])
-   net: add `tos` and `set_tos` methods to TCP and UDP sockets ([#&#8203;4877])
-   net: add security flags to named pipe `ServerOptions` ([#&#8203;4845])
-   signal: add more windows signal handlers ([#&#8203;4924])
-   sync: add `mpsc::Sender::max_capacity` method ([#&#8203;4904])
-   sync: implement Weak version of `mpsc::Sender` ([#&#8203;4595])
-   task: add `LocalSet::enter` ([#&#8203;4765])
-   task: stabilize `JoinSet` and `AbortHandle` ([#&#8203;4920])
-   tokio: add `track_caller` to public APIs ([#&#8203;4805], [#&#8203;4848], [#&#8203;4852])
-   wasm: initial support for `wasm32-wasi` target ([#&#8203;4716])

##### Fixed

-   miri: improve miri compatibility by avoiding temporary references in `linked_list::Link` impls ([#&#8203;4841])
-   signal: don't register write interest on signal pipe ([#&#8203;4898])
-   sync: add `#[must_use]` to lock guards ([#&#8203;4886])
-   sync: fix hang when calling `recv` on closed and reopened broadcast channel ([#&#8203;4867])
-   task: propagate attributes on task-locals ([#&#8203;4837])

##### Changed

-   fs: change panic to error in `File::start_seek` ([#&#8203;4897])
-   io: reduce syscalls in `poll_read` ([#&#8203;4840])
-   process: use blocking threadpool for child stdio I/O ([#&#8203;4824])
-   signal: make `SignalKind` methods const ([#&#8203;4956])

##### Internal changes

-   rt: extract `basic_scheduler::Config` ([#&#8203;4935])
-   rt: move I/O driver into `runtime` module ([#&#8203;4942])
-   rt: rename internal scheduler types ([#&#8203;4945])

##### Documented

-   chore: fix typos and grammar ([#&#8203;4858], [#&#8203;4894], [#&#8203;4928])
-   io: fix typo in `AsyncSeekExt::rewind` docs ([#&#8203;4893])
-   net: add documentation to `try_read()` for zero-length buffers ([#&#8203;4937])
-   runtime: remove incorrect panic section for `Builder::worker_threads` ([#&#8203;4849])
-   sync: doc of `watch::Sender::send` improved ([#&#8203;4959])
-   task: add cancel safety docs to `JoinHandle` ([#&#8203;4901])
-   task: expand on cancellation of `spawn_blocking` ([#&#8203;4811])
-   time: clarify that the first tick of `Interval::tick` happens immediately ([#&#8203;4951])

##### Unstable

-   rt: add unstable option to disable the LIFO slot ([#&#8203;4936])
-   task: fix incorrect signature in `Builder::spawn_on` ([#&#8203;4953])
-   task: make `task::Builder::spawn*` methods fallible ([#&#8203;4823])

[#&#8203;4595]: tokio-rs/tokio#4595

[#&#8203;4716]: tokio-rs/tokio#4716

[#&#8203;4765]: tokio-rs/tokio#4765

[#&#8203;4805]: tokio-rs/tokio#4805

[#&#8203;4811]: tokio-rs/tokio#4811

[#&#8203;4823]: tokio-rs/tokio#4823

[#&#8203;4824]: tokio-rs/tokio#4824

[#&#8203;4837]: tokio-rs/tokio#4837

[#&#8203;4840]: tokio-rs/tokio#4840

[#&#8203;4841]: tokio-rs/tokio#4841

[#&#8203;4845]: tokio-rs/tokio#4845

[#&#8203;4848]: tokio-rs/tokio#4848

[#&#8203;4849]: tokio-rs/tokio#4849

[#&#8203;4852]: tokio-rs/tokio#4852

[#&#8203;4858]: tokio-rs/tokio#4858

[#&#8203;4867]: tokio-rs/tokio#4867

[#&#8203;4877]: tokio-rs/tokio#4877

[#&#8203;4882]: tokio-rs/tokio#4882

[#&#8203;4886]: tokio-rs/tokio#4886

[#&#8203;4893]: tokio-rs/tokio#4893

[#&#8203;4894]: tokio-rs/tokio#4894

[#&#8203;4897]: tokio-rs/tokio#4897

[#&#8203;4898]: tokio-rs/tokio#4898

[#&#8203;4901]: tokio-rs/tokio#4901

[#&#8203;4904]: tokio-rs/tokio#4904

[#&#8203;4920]: tokio-rs/tokio#4920

[#&#8203;4924]: tokio-rs/tokio#4924

[#&#8203;4928]: tokio-rs/tokio#4928

[#&#8203;4935]: tokio-rs/tokio#4935

[#&#8203;4936]: tokio-rs/tokio#4936

[#&#8203;4937]: tokio-rs/tokio#4937

[#&#8203;4942]: tokio-rs/tokio#4942

[#&#8203;4945]: tokio-rs/tokio#4945

[#&#8203;4951]: tokio-rs/tokio#4951

[#&#8203;4953]: tokio-rs/tokio#4953

[#&#8203;4956]: tokio-rs/tokio#4956

[#&#8203;4959]: tokio-rs/tokio#4959

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about these updates again.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, click this checkbox.

---

This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).
<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzMi4xODcuMCIsInVwZGF0ZWRJblZlciI6IjMyLjE4Ny4wIn0=-->

Co-authored-by: cabr2-bot <cabr2.help@gmail.com>
Reviewed-on: https://codeberg.org/Calciumdibromid/CaBr2/pulls/1532
Reviewed-by: crapStone <crapstone@noreply.codeberg.org>
Co-authored-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>
Co-committed-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>
@rkuris rkuris mentioned this pull request Dec 29, 2022
Open
@Xuanwo Xuanwo mentioned this pull request Jan 16, 2023
Merged
nomeaning777 added a commit to nomeaning777/wsld that referenced this pull request Jun 12, 2023
@nomeaning777
fix(deps): fix tokio version to 1.20
0eb04dd 
vsock connection is not working correctly because of changes in 1.21
tokio-rs/tokio#4840
@jasta jasta mentioned this pull request Jul 13, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Thomasdezeeuw Thomasdezeeuw Thomasdezeeuw approved these changes

@carllerche carllerche Awaiting requested review from carllerche

@Darksonn Darksonn Awaiting requested review from Darksonn

Assignees
No one assigned
Labels
A-tokio Area: The main tokio crate M-io Module: tokio/io
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Noah-Kennedy @Darksonn @carllerche @Thomasdezeeuw