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 read and write support #58357

Open
wants to merge 4 commits into
base: master
from

Conversation

Projects
None yet
6 participants
@sfackler
Copy link
Member

sfackler commented Feb 10, 2019

This functionality has lived for a while in the tokio ecosystem, where
it can improve performance by minimizing copies.

r? @alexcrichton

@sfackler

This comment has been minimized.

Copy link
Member Author

sfackler commented Feb 10, 2019

Unix also supports vectored IO on files/stdout/etc, but I haven't plumbed that through yet.

@sfackler

This comment has been minimized.

Copy link
Member Author

sfackler commented Feb 10, 2019

@sfackler sfackler force-pushed the sfackler:vectored-io branch 2 times, most recently from 8932b55 to 03fc79a Feb 10, 2019

@carllerche

This comment has been minimized.

Copy link
Member

carllerche commented Feb 10, 2019

Seems a bit early to commit this to std. It looks like it is copying the API from iovec that has not yet been released (and has not yet gotten real world usage yet).

@sfackler

This comment has been minimized.

Copy link
Member Author

sfackler commented Feb 10, 2019

It'll be landing as unstable so we can continue to tweak as needed, but I don't think there'll really need to be all that much. The design space here is pretty constrained, and when talking about this at the all hands last week, we were all in agreement that the current iovec approach of DST hackery to avoid separate immutable and mutable types wasn't worth it. The other possibilities are this and IoVec<&[u8]>/IoVec<&mut [u8]>.

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Feb 11, 2019

This looks great to me, thanks @sfackler! I think I personally prefer the current approach over IoVec<T>.

One thing I think though we'll still want to game out is the extension traits for Unix/Windows perhaps? Ideally those would be used internally as well to avoid pointer casts

/// # Panics
///
/// Panics on Windows if the slice is larger than 4GB.
#[unstable(feature = "iovec", issue = "0")]

This comment has been minimized.

@alexcrichton

alexcrichton Feb 11, 2019

Member

Just a reminder to fill out the issue before merging

Show resolved Hide resolved src/libstd/io/mod.rs Outdated
@sfackler

This comment has been minimized.

Copy link
Member Author

sfackler commented Feb 11, 2019

The extension trait would be on [IoVec] and [IoVecMut], right?

@sfackler

This comment has been minimized.

Copy link
Member Author

sfackler commented Feb 11, 2019

Oh, we may not actually want those extension traits, since we'd need to expose iovec and WSABUF in the std public API.

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Feb 11, 2019

We don't necessarily need an extension trait but rather a free function. Thinking about it though a pointer cast is likely required anyway because the libc types on crates.io won't unify with the ones from libstd. In that case documenting this is probably the best way to go

@sfackler

This comment has been minimized.

Copy link
Member Author

sfackler commented Feb 12, 2019

I just changed them to implement Deref/DerefMut. It makes the code a bit nicer.

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented Feb 12, 2019

The job x86_64-gnu-llvm-6.0 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
travis_time:end:0fe6e128:start=1549942434637732500,finish=1549942508737713414,duration=74099980914
$ git checkout -qf FETCH_HEAD
travis_fold:end:git.checkout

Encrypted environment variables have been removed for security reasons.
See https://docs.travis-ci.com/user/pull-requests/#pull-requests-and-security-restrictions
$ export SCCACHE_BUCKET=rust-lang-ci-sccache2
$ export SCCACHE_REGION=us-west-1
Setting environment variables from .travis.yml
$ export IMAGE=x86_64-gnu-llvm-6.0
---
travis_time:start:test_debuginfo
Check compiletest suite=debuginfo mode=debuginfo-both (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
[01:07:12] 
[01:07:12] running 119 tests
[01:07:36] .iiiii...i.....i..i...i..i.i..i.ii...i.....i..i....i..........iiii..........i...ii...i.......ii.i.i. 100/119
[01:07:40] i......iii.i.....ii
[01:07:40] 
[01:07:40]  finished in 28.435
[01:07:40] travis_fold:end:test_debuginfo

---
[01:32:55] travis_fold:end:stage0-linkchecker

[01:32:55] travis_time:end:stage0-linkchecker:start=1549948090938071448,finish=1549948093129082597,duration=2191011149

[01:32:57] std/io/struct.IoVec.html:393: broken link fragment `#method.sort_by_key` pointing to `std/io/struct.IoVec.html`
[01:32:57] std/io/struct.IoVec.html:505: broken link fragment `#method.make_ascii_uppercase` pointing to `std/io/struct.IoVec.html`
[01:32:57] std/io/struct.IoVec.html:510: broken link fragment `#method.make_ascii_lowercase` pointing to `std/io/struct.IoVec.html`
[01:33:03] thread 'main' panicked at 'found some broken links', src/tools/linkchecker/main.rs:41:9
[01:33:03] 
[01:33:03] 
[01:33:03] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/linkchecker" "/checkout/obj/build/x86_64-unknown-linux-gnu/doc"
[01:33:03] expected success, got: exit code: 101
[01:33:03] expected success, got: exit code: 101
[01:33:03] 
[01:33:03] 
[01:33:03] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test
[01:33:03] Build completed unsuccessfully in 0:37:12
[01:33:03] Makefile:48: recipe for target 'check' failed
[01:33:03] make: *** [check] Error 1
The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:0852ae14
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)
Tue Feb 12 05:08:21 UTC 2019
---
travis_time:end:1303481a:start=1549948103091812517,finish=1549948103146636412,duration=54823895
travis_fold:end:after_failure.4
travis_fold:start:after_failure.5
travis_time:start:0074ab6a
$ cat ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers || true
cat: ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers: No such file or directory
travis_fold:end:after_failure.5
travis_fold:start:after_failure.6
travis_time:start:121e15f4
$ dmesg | grep -i kill

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

@sfackler

This comment has been minimized.

Copy link
Member Author

sfackler commented Feb 12, 2019

Uh, why are doc links broken?

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Feb 12, 2019

I suspect that has to do with deref inlining methods or something like that, although beyond that I'm not sure why the links are broken

In any case r=me with a tracking issue filed

@sfackler

This comment has been minimized.

Copy link
Member Author

sfackler commented Feb 13, 2019

@QuietMisdreavus do you know what's going on with the link checker up above?

sfackler added some commits Feb 8, 2019

Add vectored read and write support
This functionality has lived for a while in the tokio ecosystem, where
it can improve performance by minimizing copies.
impl Deref/DerefMut for IoVec types
Returning &'a mut [u8] was unsound, and we may as well just have them
directly deref to their slices to make it easier to work with them.

@sfackler sfackler force-pushed the sfackler:vectored-io branch from b3b5eaf to 034de8d Feb 14, 2019

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented Feb 14, 2019

The job x86_64-gnu-llvm-6.0 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
travis_time:end:02c7b877:start=1550117567041456961,finish=1550117567964715854,duration=923258893
$ git checkout -qf FETCH_HEAD
travis_fold:end:git.checkout

Encrypted environment variables have been removed for security reasons.
See https://docs.travis-ci.com/user/pull-requests/#pull-requests-and-security-restrictions
$ export SCCACHE_BUCKET=rust-lang-ci-sccache2
$ export SCCACHE_REGION=us-west-1
Setting environment variables from .travis.yml
$ export IMAGE=x86_64-gnu-llvm-6.0
---
travis_time:start:tidy
tidy check
[00:03:50] * 565 error codes
[00:03:50] * highest error code: E0722
[00:03:50] tidy error: /checkout/src/libstd/io/mod.rs:285: mismatches the `issue` in previous
[00:03:51] some tidy checks failed
[00:03:51] 
[00:03:51] 
[00:03:51] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/tidy" "/checkout/src" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "--no-vendor" "--quiet"
[00:03:51] 
[00:03:51] 
[00:03:51] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test src/tools/tidy
[00:03:51] Build completed unsuccessfully in 0:00:45
[00:03:51] Build completed unsuccessfully in 0:00:45
[00:03:51] make: *** [tidy] Error 1
[00:03:51] Makefile:68: recipe for target 'tidy' failed
The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:005b4447
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)
Thu Feb 14 04:16:49 UTC 2019
---
travis_time:end:066b627a:start=1550117809913271018,finish=1550117809917839753,duration=4568735
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:00678750
$ ln -s . checkout && for CORE in obj/cores/core.*; do EXE=$(echo $CORE | sed 's|obj/cores/core\.[0-9]*\.!checkout!\(.*\)|\1|;y|!|/|'); if [ -f "$EXE" ]; then printf travis_fold":start:crashlog\n\033[31;1m%s\033[0m\n" "$CORE"; gdb --batch -q -c "$CORE" "$EXE" -iex 'set auto-load off' -iex 'dir src/' -iex 'set sysroot .' -ex bt -ex q; echo travis_fold":"end:crashlog; fi; done || true
travis_fold:end:after_failure.4
travis_fold:start:after_failure.5
travis_time:start:2386af50
travis_time:start:2386af50
$ cat ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers || true
cat: ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers: No such file or directory
travis_fold:end:after_failure.5
travis_fold:start:after_failure.6
travis_time:start:144d6b9c
$ dmesg | grep -i kill

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

@sfackler sfackler force-pushed the sfackler:vectored-io branch from 9c76f7b to 5ca3b00 Feb 14, 2019

@sfackler

This comment has been minimized.

Copy link
Member Author

sfackler commented Feb 14, 2019

@bors r=alexcrichton

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Feb 14, 2019

📌 Commit 5ca3b00 has been approved by alexcrichton

///
/// The default implementation simply passes the first nonempty buffer to
/// `read`.
#[unstable(feature = "iovec", issue = "58452")]

This comment has been minimized.

@Amanieu

Amanieu Feb 14, 2019

Contributor

I think this could use a sentence to clarify what the return value is (i.e. the total number of bytes reads across all buffers).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment