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

Add vectored positioned I/O on Unix #89518

Merged
merged 5 commits into from
Mar 4, 2023

Conversation

a1phyr
Copy link
Contributor

@a1phyr a1phyr commented Oct 4, 2021

Add methods for vectored I/O with an offset on File for unix under #![feature(unix_file_vectored_at)].

The new methods are wrappers around preadv and pwritev.

Tracking issue: #89517

@rust-highfive
Copy link
Collaborator

r? @Mark-Simulacrum

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 4, 2021
@rust-log-analyzer

This comment has been minimized.

@joshtriplett joshtriplett added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Oct 4, 2021
@jhwgh1968
Copy link

Drive by random comment.

On line 135:

bufs.as_ptr() as *const libc::iovec

Shouldn't that be *mut libc::iovec since it is passed as &mut?

@a1phyr
Copy link
Contributor Author

a1phyr commented Oct 9, 2021

Well, I copied read_vectored for this.

Anyway, readv takes a const struct iovec*, so this is the good type. However, according to Rust rules we cannot modify something behind a shared reference, so I think we should get a *mut pointer and cast it to *const.

@Mark-Simulacrum
Copy link
Member

Seems reasonable enough. r? @joshtriplett for T-libs-api review as new unstable API

@joshtriplett
Copy link
Member

This seems reasonable to me.

I think we may want ReadBuf versions of all of these in the future, but that can be a separate PR.

@bors r+

@bors
Copy link
Contributor

bors commented Oct 11, 2021

📌 Commit 2265bef has been approved by joshtriplett

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 11, 2021
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Oct 11, 2021
…htriplett

Add vectored positioned I/O on Unix

Add methods for vectored I/O with an offset on `File` for `unix` under `#![feature(unix_file_vectored_at)]`.

The new methods are wrappers around `preadv` and `pwritev`.

Tracking issue: rust-lang#89517
@matthiaskrgr
Copy link
Member

Build failed in a rollup ( #89763 (comment) )

[RUSTC-TIMING] core test:false 15.649
[RUSTC-TIMING] addr2line test:false 0.427
[RUSTC-TIMING] gimli test:false 4.080
[RUSTC-TIMING] object test:false 4.120
error[E0425]: cannot find function `preadv` in crate `libc`
   --> library/std/src/sys/unix/fd.rs:133:19
133 |             libc::preadv(
    |                   ^^^^^^ help: a function with a similar name exists: `pread`
    |
   ::: /cargo/registry/src/github.com-1ecc6299db9ec823/libc-0.2.103/src/unix/mod.rs:933:5
   ::: /cargo/registry/src/github.com-1ecc6299db9ec823/libc-0.2.103/src/unix/mod.rs:933:5
    |
933 |     pub fn pread(fd: ::c_int, buf: *mut ::c_void, count: ::size_t, offset: off_t) -> ::ssize_t;
    |     ------------------------------------------------------------------------------------------- similarly named function `pread` defined here

error[E0425]: cannot find function `pwritev` in crate `libc`
   --> library/std/src/sys/unix/fd.rs:213:19
    |
213 |             libc::pwritev(
    |                   ^^^^^^^ help: a function with a similar name exists: `pwrite`
   ::: /cargo/registry/src/github.com-1ecc6299db9ec823/libc-0.2.103/src/unix/mod.rs:938:5
    |
    |
938 |     pub fn pwrite(fd: ::c_int, buf: *const ::c_void, count: ::size_t, offset: off_t) -> ::ssize_t;
    |     ---------------------------------------------------------------------------------------------- similarly named function `pwrite` defined here
For more information about this error, try `rustc --explain E0425`.
[RUSTC-TIMING] std test:false 1.946
error: could not compile `std` due to 2 previous errors
Build completed unsuccessfully in 0:15:29

@bors r-

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Oct 11, 2021
@a1phyr
Copy link
Contributor Author

a1phyr commented Oct 11, 2021

I figured out that preadv/pwritev are not POSIX, even if they are defined by most UNIX platforms.

Now preadv/pwritev are used it on all platform for which libc defines them, with a fallback implementation for other platforms.

@JohnCSimon JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 31, 2021
@JohnCSimon JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 5, 2021
@bors
Copy link
Contributor

bors commented Dec 9, 2021

☔ The latest upstream changes (presumably #81156) made this pull request unmergeable. Please resolve the merge conflicts.

@a1phyr a1phyr force-pushed the unix_file_vectored_at branch from e7967df to 00de714 Compare December 13, 2021 11:01
@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Dec 13, 2021
@a1phyr
Copy link
Contributor Author

a1phyr commented Dec 13, 2021

Rebased

@a1phyr
Copy link
Contributor Author

a1phyr commented Mar 1, 2023

Wild guess: preadv was added to bionic in 2015 (cf https://android.googlesource.com/platform/bionic/+/6f4594d). The tests may run with on older version.

To fix this, we could either disable preadv on android or use weak linkage.

@workingjubilee
Copy link
Member

Our Android minimum is increasing to Android KitKat. That's lower than Marshmallow, the Oct 2015 release. I need more coffee before I remember the details of how weak symbol linkage works, but what we do here has to work on KitKat and Lollipop, so go with whichever feels right there.

@a1phyr
Copy link
Contributor Author

a1phyr commented Mar 2, 2023

I edited the PR to:

  • Use weak linkage on Android (with fallback to syscall on 64 bits systems)
  • Use &self in the public API
  • Add basic tests to make sure everything works as intended enverywhere

@rustbot label -S-waiting-on-author +S-waiting-on-review

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 2, 2023
Copy link
Member

@workingjubilee workingjubilee left a comment

Choose a reason for hiding this comment

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

Bad news, I looked it up and it seems this was only introduced relatively late in the game by macOS (and iOS). It seems even #104385 won't raise the minimum supported versions high enough to guarantee pwritev on those platforms.

library/std/src/sys/unix/fd.rs Show resolved Hide resolved
@a1phyr a1phyr force-pushed the unix_file_vectored_at branch from 03afae1 to d760da4 Compare March 3, 2023 09:35
@a1phyr a1phyr requested a review from workingjubilee March 3, 2023 09:39
@a1phyr a1phyr force-pushed the unix_file_vectored_at branch from d760da4 to 92f35b3 Compare March 3, 2023 09:40
Copy link
Member

@workingjubilee workingjubilee left a comment

Choose a reason for hiding this comment

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

Huzzah, let's see if this goes or if it tells us to do more.

@bors r+

@workingjubilee
Copy link
Member

Not again?
@bors r+

@bors
Copy link
Contributor

bors commented Mar 3, 2023

📌 Commit 92f35b3 has been approved by workingjubilee

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 3, 2023
@bors
Copy link
Contributor

bors commented Mar 4, 2023

⌛ Testing commit 92f35b3 with merge 0fbfc3e...

@bors
Copy link
Contributor

bors commented Mar 4, 2023

☀️ Test successful - checks-actions
Approved by: workingjubilee
Pushing 0fbfc3e to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Mar 4, 2023
@bors bors merged commit 0fbfc3e into rust-lang:master Mar 4, 2023
@rustbot rustbot added this to the 1.69.0 milestone Mar 4, 2023
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (0fbfc3e): comparison URL.

Overall result: ❌ regressions - ACTION NEEDED

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression
cc @rust-lang/wg-compiler-performance

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
0.2% [0.2%, 0.3%] 4
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 0.2% [0.2%, 0.3%] 4

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
1.3% [1.3%, 1.3%] 1
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-5.7% [-5.7%, -5.7%] 1
Improvements ✅
(secondary)
-2.9% [-3.0%, -2.7%] 2
All ❌✅ (primary) -2.2% [-5.7%, 1.3%] 2

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
0.8% [0.6%, 1.0%] 8
Regressions ❌
(secondary)
0.8% [0.8%, 0.8%] 1
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 0.8% [0.6%, 1.0%] 8

@rustbot rustbot added the perf-regression Performance regression. label Mar 4, 2023
@rylev
Copy link
Member

rylev commented Mar 7, 2023

Small regression in Diesel which has started to show some bimodality. I think it's safe to call this triaged.

@rustbot label +perf-regression-triaged

@rustbot rustbot added the perf-regression-triaged The performance regression has been triaged. label Mar 7, 2023
@a1phyr a1phyr deleted the unix_file_vectored_at branch March 12, 2024 17:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. O-unix Operating system: Unix-like perf-regression Performance regression. perf-regression-triaged The performance regression has been triaged. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.