Skip to content

cat: resize the output pipe once per invocation - #15506

Open
limadog9 wants to merge 1 commit into
uutils:mainfrom
limadog9:fix/cat-resize-output-pipe-once-11700
Open

limadog9 wants to merge 1 commit into
uutils:mainfrom
limadog9:fix/cat-resize-output-pipe-once-11700

Conversation

@limadog9

@limadog9 limadog9 commented Oct 18, 2026 •

Copy link
Copy Markdown
Contributor

Prepare stdout's pipe capacity once and reuse the broker copy for every input. On 30 files, resize attempts fall from 31 to 2. A strace regression checks one broker pipe and two resize attempts with cat /dev/null /dev/ null.

Closes #11700.

Comment thread src/uu/cat/Cargo.toml

[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.

Comment thread src/uucore/src/lib/features/pipes.rs Outdated
Comment on lines +108 to +119
// Increasing the output pipe capacity is optional and may be done once by
// callers that copy several inputs into the same output.
let _ = fcntl_setpipe_size(&mut *dest, MAX_ROOTLESS_PIPE_SIZE);
splice_unbounded_broker(source, dest)
}

/// Copy through a cached broker pipe without changing the output pipe capacity.
///
/// Callers copying multiple sources may prepare their output once and reuse
/// this operation for each source. A failure to resize never prevents copying.
#[inline]
pub fn splice_unbounded_broker(source: &impl AsFd, dest: &mut impl AsFd) -> SpliceRes {

This comment was marked as low quality.

@oech3

This comment was marked as low quality.

Comment on lines -112 to -114
// fcntl for input would not improve throughput since
// - sender with splice probably increased size already
// - sender without splice is bottleneck

This comment was marked as resolved.

@limadog9
limadog9 force-pushed the fix/cat-resize-output-pipe-once-11700 branch from c4d75cf to 34525a4 Compare October 18, 2026 19:57
Comment thread src/uu/cat/src/cat.rs Outdated
Comment on lines +415 to +416
// All operands share stdout. Resize its pipe once, even if the kernel
// refuses the optional increase, rather than retrying for every file.

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.

Suggested change
// All operands share stdout. Resize its pipe once, even if the kernel
// refuses the optional increase, rather than retrying for every file.
// All operands share stdout. Try to resize it once.

@limadog9
limadog9 force-pushed the fix/cat-resize-output-pipe-once-11700 branch from 34525a4 to c7f35d0 Compare October 18, 2026 20:40
Comment thread src/uu/cat/src/cat.rs
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.

Comment thread src/uucore/src/lib/features/pipes.rs Outdated
/// Copy through a cached broker pipe without changing the output pipe capacity.
///
/// Callers copying multiple sources may prepare their output once and reuse
/// this operation for each source. A failure to resize never prevents copying.

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.

this function never resizes, so "a failure to resize never prevents copying" is confusing here. could you please make this doc one line?

@oech3 oech3 Oct 18, 2026 •

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 think we don't need this comment.

@limadog9
limadog9 force-pushed the fix/cat-resize-output-pipe-once-11700 branch from c7f35d0 to 736f4f1 Compare October 18, 2026 22:29
@codspeed

codspeed Bot commented Oct 19, 2026 •

Copy link
Copy Markdown

Merging this PR will not alter performance

⚠️ 119 benchmarks spent significant time in system calls

System calls cannot be consistently instrumented, so they are not included in the measure, which understates the real cost. Please switch to the Walltime instrument to accurately measure system calls.

Measurement and system calls

ℹ️ 1 benchmark has no flame graph

The flamegraph could not be generated because too much time was spent in runtime internals, making the visualization not useful for performance analysis.

✅ 379 untouched benchmarks
⏩ 92 skipped benchmarks1


Comparing limadog9:fix/cat-resize-output-pipe-once-11700 (74bd78d) with main (d2d9694)

Open in CodSpeed

Footnotes

  1. 92 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩

@limadog9
limadog9 force-pushed the fix/cat-resize-output-pipe-once-11700 branch from 736f4f1 to adfcce3 Compare October 19, 2026 10:06
@limadog9

Copy link
Copy Markdown
Contributor Author

Added the requested strace regression

Comment thread tests/by-util/test_cat.rs Outdated
}

#[test]
fn test_fast_copy_mixed_file_and_stdin_order() {

@oech3 oech3 Oct 19, 2026 •

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.

Would you remove unrelated test and update 1st comment to mention strace test instead?

Comment thread tests/by-util/test_cat.rs Outdated

let scene = TestScenario::new(util_name!());
let probe = Command::new("strace")
.args(["-qq", "-o", "/dev/null", "-e", "trace=none"])

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 think we can check support of syscalls by kernel at here instead of trace=none.

Comment thread tests/by-util/test_cat.rs Outdated
let probe = Command::new("strace")
.args(["-qq", "-o", "/dev/null", "-e", "trace=none"])
.arg(&scene.bin_path)
.args(["cat", "--help"])

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 think --version is faster, or cat /dev/null is making a sense.

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.

Moreover, we don't need 2 strace at here if we used cat /dev/null /dev/null.

Prepare stdout pipe capacity once and reuse the broker copy for every
input. On 30 files, resize attempts fall from 31 to 2.

Add a strace regression using cat /dev/null /dev/null to check one broker
pipe and two resize attempts. Detect unavailable syscalls from the same
trace rather than launching a separate probe.

Closes uutils#11700.
@limadog9
limadog9 force-pushed the fix/cat-resize-output-pipe-once-11700 branch from adfcce3 to 74bd78d Compare October 19, 2026 10:43

This branch has not been deployed

No deployments
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.

cat: overkilled fcntl() call with multiple input

3 participants