Skip to content

tee: merge flush into write for simplicity - #12089

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

tee: merge flush into write for simplicity#12089
oech3 wants to merge 1 commit into
uutils:mainfrom
oech3:tee-flush

Conversation

@oech3

@oech3 oech3 commented Apr 30, 2026

Copy link
Copy Markdown
Contributor

Then we can avoid let res = (|| { *** })();.

@github-actions

github-actions Bot commented Apr 30, 2026

Copy link
Copy Markdown

GNU testsuite comparison:

Skipping an intermittent issue tests/date/date-locale-hour (passes in this run but fails in the 'main' branch)
Congrats! The gnu test tests/cut/bounded-memory is now passing!
Congrats! The gnu test tests/printf/printf-surprise is now passing!

@xtqqczze

xtqqczze commented Apr 30, 2026

Copy link
Copy Markdown
Contributor

I don't see the benefit of this change, it also means we won't be able to extract Writer to uucore::writable.

@oech3

oech3 commented Apr 30, 2026

Copy link
Copy Markdown
Contributor Author

I believe that there is an advantage to do that more than moving this to uucore since

  • posix tee requirement is specialized compared with other utils
  • let res = ... is nightmare when I call write_all at another place

@oech3

oech3 commented Apr 30, 2026

Copy link
Copy Markdown
Contributor Author

I guess we already have same things on comm and unexpand we can move to uucore.

@codspeed-hq

codspeed-hq Bot commented Apr 30, 2026

Copy link
Copy Markdown

Merging this PR will degrade performance by 3.13%

⚠️ Different runtime environments detected

Some benchmarks with significant performance changes were compared across different runtime environments,
which may affect the accuracy of the results.

Open the report in CodSpeed to investigate

❌ 1 regressed benchmark
✅ 310 untouched benchmarks
⏩ 46 skipped benchmarks1

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Performance Changes

Mode Benchmark BASE HEAD Efficiency
Simulation du_max_depth_balanced_tree[(6, 4, 10)] 25.1 ms 25.9 ms -3.13%

Comparing oech3:tee-flush (d756c7a) with main (5316f58)

Open in CodSpeed

Footnotes

  1. 46 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.

@oech3
oech3 marked this pull request as ready for review April 30, 2026 02:49
@xtqqczze

xtqqczze commented Apr 30, 2026

Copy link
Copy Markdown
Contributor

I guess we already have same things on comm and unexpand we can move to uucore.

We could use Writer in place of Box<dyn Write> elsewhere to avoid dynamic dispatch.

@oech3

oech3 commented Apr 30, 2026

Copy link
Copy Markdown
Contributor Author

tee does not require fn write anyway. So we don't need those code on tee.rs.
We can recreate this at uucore if needed.

@xtqqczze

Copy link
Copy Markdown
Contributor

It doesn't make sense to replace idiomatic Write implementation just to avoid one instance of let res = (|| { *** })();.

@oech3

oech3 commented Apr 30, 2026 via email

Copy link
Copy Markdown
Contributor Author

@oech3

oech3 commented Apr 30, 2026

Copy link
Copy Markdown
Contributor Author

Also this is the only way to avoid executing 2 match.

@xtqqczze

xtqqczze commented May 3, 2026

Copy link
Copy Markdown
Contributor

I don't see the benefit of this change, it also means we won't be able to extract Writer to uucore::writable.

The change makes sense in context of #12131.

@oech3

oech3 commented May 3, 2026

Copy link
Copy Markdown
Contributor Author

The change makes sense in context of #12131.

However, it removed flush(). So let res = (|| { *** })(); can be removed. I'm not sure what should I do at here.
Maybe, Widows does not have buffering on std::io?

@oech3
oech3 marked this pull request as draft May 3, 2026 15:35
@oech3

oech3 commented May 3, 2026

Copy link
Copy Markdown
Contributor Author

It seems line buffering of stdout is unix specific. So I think flush can be just removed at #12131 .

@oech3

oech3 commented May 3, 2026

Copy link
Copy Markdown
Contributor Author

I saw

failures:
    test_tee::test_tee_output_not_buffered

on Windows if I removed flush...

Comment thread src/uu/tee/src/tee.rs
@oech3

oech3 commented May 5, 2026

Copy link
Copy Markdown
Contributor Author

drained by #12131

@oech3 oech3 closed this May 5, 2026
@oech3
oech3 deleted the tee-flush branch May 5, 2026 11:12
@oech3
oech3 restored the tee-flush branch May 12, 2026 16:22
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.

2 participants