Skip to content

tee: merge logic of flush into write#11914

Closed
oech3 wants to merge 1 commit intouutils:mainfrom
oech3:tee-flush
Closed

tee: merge logic of flush into write#11914
oech3 wants to merge 1 commit intouutils:mainfrom
oech3:tee-flush

Conversation

@oech3
Copy link
Copy Markdown
Contributor

@oech3 oech3 commented Apr 20, 2026

We always flush() after write. So we don't need 2 functions.
Closes #11893 .

@github-actions
Copy link
Copy Markdown

GNU testsuite comparison:

GNU test failed: tests/timeout/timeout-group. tests/timeout/timeout-group is passing on 'main'. Maybe you have to rebase?
Skip an intermittent issue tests/pr/bounded-memory (fails in this run but passes in the 'main' branch)
Skip an intermittent issue tests/tail/tail-n0f (fails in this run but passes in the 'main' branch)
Congrats! The gnu test tests/printf/printf-surprise is now passing!

@codspeed-hq
Copy link
Copy Markdown

codspeed-hq Bot commented Apr 20, 2026

Too many benchmarks in a single upload

The performance report could not be generated because there were too many benchmarks in a single upload to CodSpeed. We recommend sharding your benchmarks into smaller uploads, with max 1000 benchmarks per upload. See the documentation for more information.

@oech3 oech3 marked this pull request as ready for review April 20, 2026 09:33
@oech3
Copy link
Copy Markdown
Contributor Author

oech3 commented Apr 20, 2026

I think usize returned by write_all is not used at anywhere.

@xtqqczze
Copy link
Copy Markdown
Contributor

I've incorporated these changed into #11893

@oech3
Copy link
Copy Markdown
Contributor Author

oech3 commented Apr 20, 2026

Is it able to reduce 15 lines at your PR?

@xtqqczze
Copy link
Copy Markdown
Contributor

@codspeedbot why have we started getting "Too many benchmarks in a single upload" error when number of benchmarks haven't changed?

@codspeed-hq
Copy link
Copy Markdown

codspeed-hq Bot commented Apr 20, 2026

@codspeedbot why have we started getting "Too many benchmarks in a single upload" error when number of benchmarks haven't changed?

Based on my investigation, here's what I found:

Your repository currently has 310 benchmarks in total (155 unique benchmarks × 2 modes: simulation + memory), spread across 62 matrix jobs (31 packages × 2 modes). Each individual shard contains only 5–20 benchmarks, which is well under the 1,000 per-upload recommendation.

Looking at recent CodSpeed runs for this repository:

Since the same benchmark configuration succeeds for some runs and fails for others — and the total count is far below the 1,000 limit — this is an intermittent issue on the CodSpeed platform side, not a problem with your repository configuration.

I've flagged this for the CodSpeed engineering team to investigate. In the meantime, re-running the benchmark workflow should resolve the error for this PR.

Summary
  • Warming up the performance wizard...
  • Investigating CodSpeed error — Analysis complete. The benchmark count (310 total) is well within limits. This appears to be a platform-side intermittent issue.
  • Handling comment — Comment handling finished successfully

@cakebaker
Copy link
Copy Markdown
Contributor

cakebaker commented Apr 20, 2026

Hm, I'm confused. Do I understand it correctly that this PR is a spin-off from #11893 and should be merged before #11893?

@oech3
Copy link
Copy Markdown
Contributor Author

oech3 commented Apr 20, 2026 via email

Comment thread src/uu/tee/src/tee.rs
self.writers.retain_mut(|writer| {
writer
.write_all(buf)
.and_then(|_| writer.flush()) // avoid buffering
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 pattern is unidiomatic, see #11919

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Would you provide more detail?

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.

Specifically the unused arg for and_then

Copy link
Copy Markdown
Contributor Author

@oech3 oech3 Apr 20, 2026

Choose a reason for hiding this comment

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

But this very reasonable way to chain flush. Is there any alternative without a bunch of dupes?

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.

See #11893

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

let res = (|| {
    writer.write_all(buf)?;
    writer.flush()?;
    Ok(())
})();

is long...

Copy link
Copy Markdown
Contributor

@xtqqczze xtqqczze Apr 20, 2026

Choose a reason for hiding this comment

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

Can be reduced to:

(|| {
    writer.write_all(buf)?;
    writer.flush()
})()

Once try_blocks are stabilised, this becomes cleaner:

try {
    writer.write_all(buf)?;
    writer.flush()?;
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Can we keep chain with the form?

@GuillaumeLagrange
Copy link
Copy Markdown

@codspeedbot why have we started getting "Too many benchmarks in a single upload" error when number of benchmarks haven't changed?

@xtqqczze the codspeed failures are due to something that changed on our end. We have temporarily reverted the changes while we fix the issue. Retrying your runs should now succeed. Sorry for this

@codspeed-hq
Copy link
Copy Markdown

codspeed-hq Bot commented Apr 20, 2026

@codspeedbot why have we started getting "Too many benchmarks in a single upload" error when number of benchmarks haven't changed?

@xtqqczze the codspeed failures are due to something that changed on our end. We have temporarily reverted the changes while we fix the issue. Retrying your runs should now succeed. Sorry for this

This comment is an informational update from the CodSpeed team — no code or configuration changes are needed in the repository.

To summarize for @xtqqczze: the "Too many benchmarks in a single upload" error was caused by a temporary change on the CodSpeed platform side and has since been reverted. Re-running the CI workflows on this PR should now produce the performance report successfully.

Summary
  • Warming up the performance wizard...
  • Assessing request — No action needed — this is an informational update, not an actionable request
  • Handling comment — Comment handling finished successfully

@cakebaker
Copy link
Copy Markdown
Contributor

@GuillaumeLagrange thanks for the quick "fix"!

@oech3
Copy link
Copy Markdown
Contributor Author

oech3 commented Apr 20, 2026

@xtqqczze Ok. @cakebaker Please ignore this PR and merge #11893 . Sorry for confusing you.

@oech3 oech3 closed this Apr 20, 2026
@oech3 oech3 deleted the tee-flush branch April 20, 2026 16:23
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.

4 participants