Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/uu/cat/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ tempfile = { workspace = true }
uucore = { workspace = true, features = ["benchmark"] }

[target.'cfg(unix)'.dependencies]
rustix = { workspace = true, features = ["fs"] }
rustix = { workspace = true, features = ["fs", "pipe"] }

This comment was marked as low quality.

This comment was marked as low quality.


[target.'cfg(windows)'.dependencies]
windows-sys = { workspace = true, features = ["Win32_Storage_FileSystem"] }
Expand Down
8 changes: 7 additions & 1 deletion src/uu/cat/src/cat.rs
Original file line number Diff line number Diff line change
Expand Up @@ -410,6 +410,12 @@ fn cat_files<'a, I>(files: I, options: &OutputOptions) -> UResult<()>
where
I: IntoIterator<Item = &'a OsString>,
{
#[cfg(any(target_os = "linux", target_os = "android"))]
if options.can_print_fast() {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

the description mentions a regression test for mixed file/stdin order but i don't see it in the commit, please add it in tests/by-util/test_cat.rs

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't think this PR needs a test since it is performance-related PR.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If we test something, it should count pipe2 call by strace.
a regression checks mixed file/stdin output order is unrelated with this PR.

// All operands share stdout. Try to resize it once.
let _ =
rustix::pipe::fcntl_setpipe_size(io::stdout(), uucore::pipes::MAX_ROOTLESS_PIPE_SIZE);
}
let mut state = OutputState {
line_number: LineNumber::new(),
at_line_start: true,
Expand Down Expand Up @@ -496,7 +502,7 @@ fn print_fast<R: FdReadable>(handle: &mut InputHandle<R>) -> CatResult<()> {
let mut stdout = stdout;
// Try to use the splice() system call for faster writing. If it works, we're done.
#[cfg(any(target_os = "linux", target_os = "android"))]
if uucore::pipes::splice_unbounded_auto(&handle.reader, &mut stdout)?.is_ok() {
if uucore::pipes::splice_unbounded_broker(&handle.reader, &mut stdout)?.is_ok() {
return Ok(());
}

Expand Down
14 changes: 10 additions & 4 deletions src/uucore/src/lib/features/pipes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -105,14 +105,20 @@ pub fn drain_pipe(pipe: &PipeReader, dest: &impl AsFd, len: usize) -> PipeRes {
/// This includes read ahead and optimization for stdout's pipe size
#[inline]
pub fn splice_unbounded_auto(source: &impl AsFd, dest: &mut impl AsFd) -> SpliceRes {
let Ok((pipe_rd, pipe_wr)) = PIPE_CACHE.take().map_or_else(pipe::<false>, Ok) else {
return Ok(Err(()));
};

// fcntl for input would not improve throughput since
// - sender with splice probably increased size already
// - sender without splice is bottleneck
let _ = fcntl_setpipe_size(&mut *dest, MAX_ROOTLESS_PIPE_SIZE);
splice_unbounded_broker(source, dest)
}

/// Copy through a cached broker pipe without resizing the output pipe.
#[inline]
pub fn splice_unbounded_broker(source: &impl AsFd, dest: &mut impl AsFd) -> SpliceRes {
let Ok((pipe_rd, pipe_wr)) = PIPE_CACHE.take().map_or_else(pipe::<false>, Ok) else {
return Ok(Err(()));
};

// pre-generate page caches for splice
let _ = rustix::fs::fadvise(source, 0, None, rustix::fs::Advice::Sequential);
// 1st error is used to detect missing support for splice
Expand Down
37 changes: 37 additions & 0 deletions tests/by-util/test_cat.rs
Original file line number Diff line number Diff line change
Expand Up @@ -974,3 +974,40 @@ fn test_closed_stdin() {
.stdout_is("content\n")
.stderr_is("cat: -: Bad file descriptor\n");
}

#[test]
#[cfg(all(target_os = "linux", not(wasi_runner)))]
fn test_fast_copy_reuses_pipe_across_files() {
use std::process::Command;

let scene = TestScenario::new(util_name!());
let Ok(output) = Command::new("strace")
.args(["-qq", "-o", "trace", "-e", "trace=pipe2,fcntl"])
.arg(&scene.bin_path)
.args(["cat", "/dev/null", "/dev/null"])
.current_dir(scene.fixtures.as_string())
.output()
else {
eprintln!("TEST SKIPPED: strace is unavailable");
return;
};
let trace = read_to_string(scene.fixtures.plus("trace")).unwrap_or_default();
let stderr = String::from_utf8_lossy(&output.stderr);
if trace.is_empty() && stderr.starts_with("strace:") {
eprintln!("TEST SKIPPED: strace could not trace pipe syscalls: {stderr}");
return;
}
if trace.lines().any(|line| {
line.contains("ENOSYS") || (line.contains("F_SETPIPE_SZ") && line.contains("EINVAL"))
}) {
eprintln!("TEST SKIPPED: kernel does not support the pipe syscalls: {trace}");
return;
}
assert!(output.status.success(), "{output:?}");
assert!(output.stdout.is_empty(), "{output:?}");
assert!(output.stderr.is_empty(), "{output:?}");

assert_eq!(trace.matches("pipe2(").count(), 1, "{trace}");
// One attempt for stdout and one for the cached broker, regardless of operands.
assert_eq!(trace.matches("F_SETPIPE_SZ").count(), 2, "{trace}");
}
Loading