tee: increase buf size for large input#11441
Merged
sylvestre merged 1 commit intouutils:mainfrom Apr 4, 2026
Merged
Conversation
Contributor
|
The existing code is documented to use same |
Contributor
|
GNU coreutils uses |
Contributor
Author
|
Optimal value might different at each PC. 64*1024 was best for me. |
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
Contributor
This measures throughput in the most ideal scenario for a large buffer, we should ensure performance does not regress in other cases where output is small. |
Contributor
Author
|
Time for small input is bounded. But I could use smaller buf only for 1st cycle. |
Contributor
|
Please test something like: |
This comment was marked as outdated.
This comment was marked as outdated.
oech3
force-pushed
the
tee-pv
branch
3 times, most recently
from
6897cbc to
0a8e582
Compare
Contributor
Author
|
I added a code path for smaller input. There is variance at Both of win and lose happen at |
oech3
force-pushed
the
tee-pv
branch
2 times, most recently
from
7c43c1f to
dac8b97
Compare
|
GNU testsuite comparison: |
|
GNU testsuite comparison: |
Contributor
|
needs to be rebased |
oech3
changed the title
|
GNU testsuite comparison: |
|
GNU testsuite comparison: |
Contributor
@oech3 Could you quantify the performance improvement from the additional code path to justify the added complexity? |
Contributor
Author
|
This PR was opened for larger files. Mostly 0 perf change for small files. |
Contributor
|
Please add a benchmark in a different PR |
|
GNU testsuite comparison: |
Contributor
Author
|
Is it equivalint with ? |
Contributor
Author
|
How to use stdin at benchmark? |
Contributor
Author
|
Would you suggest how to use stdin at CodSpeed? |
Contributor
Author
|
We don't have bench for stream yet. Was this really OK to merge? |
Contributor
|
you did some testing, i trust you :) |
|
This broke on my Gentoo system for some reason: Reproducergentoo-reproducer.bats#!/usr/bin/env bats
good_commit="60d448be4825e5e1a4ebbb2cbb74f562a9c899ea"
bad_commit="13fb3bede8185bfe12d0579027dfd6ab13793185"
setup() {
[ -d /var/db/repos/gentoo/dev-lang/rust-bin ] || emerge-webrsync
command -v rustc || emerge -q1 dev-lang/rust-bin
command -v git || emerge -q1 dev-vcs/git
keyword_file="/etc/portage/package.accept_keywords/uutils"
[ -e "$keyword_file" ] || printf "%s **\n" sys-apps/uutils-coreutils > "$keyword_file"
[ -e /usr/bin/gnu-tee ] || cp -afv /usr/bin/tee /usr/bin/gnu-tee
}
teardown() {
echo "Restoring GNU tee"
cp -afv /usr/bin/gnu-tee /usr/bin/tee
}
@test "uucore: add missing fs feature to rustix $good_commit" {
env EGIT_OVERRIDE_COMMIT_UUTILS_COREUTILS="$good_commit" emerge -q1 uutils-coreutils
cp -afv /usr/bin/uu-tee /usr/bin/tee
emerge -v1 sys-apps/portage
}
@test "tee: increase buf size for large input $bad_commit" {
env EGIT_OVERRIDE_COMMIT_UUTILS_COREUTILS="$bad_commit" emerge -q1 uutils-coreutils
cp -afv /usr/bin/uu-tee /usr/bin/tee
emerge -v1 sys-apps/portage
}Run it like: docker run -it --rm --pull always -v $PWD/gentoo-reproducer.bats:/gentoo-reproducer.bats gentoo/stage3 bash -x -c 'cat /gentoo-reproducer.bats && emerge-webrsync && emerge -q1 dev-util/bats && bats --verbose-run --trace --formatter tap /gentoo-reproducer.bats'Output |
Contributor
Author
|
Would you provide a minimized step for breakage?
I don't have Gentoo and container.
|
|
I don't know why yet. I'll try to find out. I made a wrapper like this at /usr/bin/tee: The last thing it ran was: I don't know what was on the other end of the pipe to cause it to break though. I'll try to find it. |
|
Claude was able to spot the issue: The sanity check it came up with: (for i in $(seq 5); do echo "line $i"; sleep 0.1; done) \
| ./target/release/coreutils tee /tmp/portage_log_test.log
line 1With Busybox tee: (for i in $(seq 5); do echo "line $i"; sleep 0.1; done) \
| busybox tee /tmp/portage_log_test.log
line 1
line 2
line 3
line 4
line 5 |
oech3
commented
Apr 6, 2026
| // flush the buffer to comply with POSIX requirement that | ||
| // `tee` does not buffer the input. | ||
| output.flush()?; | ||
| return Ok(len); |
Contributor
Author
There was a problem hiding this comment.
maybe, this return caused it
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
taskset -c 1 yes|taskset -c 2 target/release/tee|taskset -c 3 pv>/dev/nullThis PR: [3.83GiB/s]
gnu58d88d243/tee: [2.83GiB/s]
closes #11432