Skip to content

[libc++] __vprint_unicode_posix() has unnecessary call to fflush() #70142

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
dimztimz opened this issue Oct 24, 2023 · 10 comments · May be fixed by #70321
Open

[libc++] __vprint_unicode_posix() has unnecessary call to fflush() #70142

dimztimz opened this issue Oct 24, 2023 · 10 comments · May be fixed by #70321
Assignees
Labels
format C++20 std::format or std::print, and anything related to them libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi.

Comments

@dimztimz
Copy link
Contributor

__vprint_unicode_posix(FILE* __stream, string_view __fmt, format_args __args, bool __write_nl, bool __is_terminal) {
// TODO PRINT Should flush errors throw too?
if (__is_terminal)
std::fflush(__stream);
__print::__vprint_nonunicode(__stream, __fmt, __args, __write_nl);
}

In this function the following two lines are not necessary and introduce performance pessimization.

if (__is_terminal)
std::fflush(__stream);

Additionally, if you read the standard carefully http://www.eel.is/c++draft/print.fun#7.sentence-4 It says:

If the native Unicode API is used, the function flushes stream before writing out.

Although POSIX offers a way to query if a C stream goes to terminal or not, it does not have native Unicode API, so such API is never used, and flushing is not needed. A consequence of this is that on POSIX querying the C stream is not needed at all and it is another performance pessimization. The call to __is_terminal() is not needed here.

# ifndef _WIN32
__print::__vprint_unicode_posix(__stream, __fmt, __args, __write_nl, __print::__is_terminal(__stream));

@github-actions github-actions bot added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label Oct 24, 2023
@dimztimz
Copy link
Contributor Author

Ping @mordante, @vitaut

@vitaut
Copy link

vitaut commented Oct 24, 2023

You are right, flush shouldn't be here.

@philnik777 philnik777 added the format C++20 std::format or std::print, and anything related to them label Oct 25, 2023
@dimztimz
Copy link
Contributor Author

The simplest fix is to delete __vprint_unicode_posix() entirely, and call __print::__vprint_nonunicode() directly from __vprint_unicode() in the case of POSIX.

@mordante
Copy link
Member

Note to self this has been introduced in #73262 too.

@dimztimz
Copy link
Contributor Author

Note to self this has been introduced in #73262 too.

As it stands now the ostream overloads do two flushes, one is the os.flush() the other is fflush(). This too should be fixed.

The correct behaviour for the ostream vprint_unicode is the following:

  • On POSIX don't call any flush or is_terminal()
  • On Windows in the case where the stream prints to terminal call only os.flush(), no need for the the C fflush(),

@mordante
Copy link
Member

Sorry for the late reply. I've been quite busy and wanted to investigate further was is considered a "native Unicode API". This is currently discussed in private e-mails but it indeed seems POSIX is considered not to have a "native Unicode API". In that case it's indeed not needed to call is_terminal nor to flush. I want to wait for that discussion to finish before moving on with the PR.

dimztimz added a commit to dimztimz/llvm-project that referenced this issue May 14, 2024
The check whether the stream is associated with a terminal or not is
needed only on Windows. The flush is needed only on Windows and when the
stream is terminal. The flushing is done only once before using the native
Unicode API on Windows.

Additionally, the correct flush is now called. In the case of C stream
overloads of print(), std::fflush() should be used, and in the
ostream overloads, only ostream::flush() member function should be used.
Before this fix, the ostream overloads called ostream::flush() and then
std::fflush().

See also https://wg21.link/LWG4044.

Fixes llvm#70142
@dimztimz
Copy link
Contributor Author

I rebased and updated the PR so it fixes the ostream overloads too.

dimztimz added a commit to dimztimz/llvm-project that referenced this issue Jul 22, 2024
The check whether the stream is associated with a terminal or not is
needed only on Windows. The flush is needed only on Windows and when the
stream is terminal. The flushing is done only once before using the native
Unicode API on Windows.

Additionally, the correct flush is now called. In the case of C stream
overloads of print(), std::fflush() should be used, and in the
ostream overloads, only ostream::flush() member function should be used.
Before this fix, the ostream overloads called ostream::flush() and then
std::fflush().

See also https://wg21.link/LWG4044.

Fixes llvm#70142
dimztimz added a commit to dimztimz/llvm-project that referenced this issue Oct 16, 2024
The check whether the stream is associated with a terminal or not is
needed only on Windows. The flush is needed only on Windows and when the
stream is terminal. The flushing is done only once before using the native
Unicode API on Windows.

Additionally, the correct flush is now called. In the case of C stream
overloads of print(), std::fflush() should be used, and in the
ostream overloads, only ostream::flush() member function should be used.
Before this fix, the ostream overloads called ostream::flush() and then
std::fflush().

See also https://wg21.link/LWG4044.

Fixes llvm#70142
dimztimz added a commit to dimztimz/llvm-project that referenced this issue Nov 23, 2024
The check whether the stream is associated with a terminal or not is
needed only on Windows. The flush is needed only on Windows and when the
stream is terminal. The flushing is done only once before using the native
Unicode API on Windows.

Additionally, the correct flush is now called. In the case of C stream
overloads of print(), std::fflush() should be used, and in the
ostream overloads, only ostream::flush() member function should be used.
Before this fix, the ostream overloads called ostream::flush() and then
std::fflush().

See also https://wg21.link/LWG4044.

Fixes llvm#70142
@adah1972
Copy link

I have recently benchmarked std::print and fmt::print, and found that the call to isatty can slow down std::print by 6x.

The benchmark test redirects the output to /dev/null, so the 6x difference only takes isatty into account, but not fflush. I can imagine it would be even worse on a real console....

@adah1972
Copy link

My benchmark result with Clang 19 on macOS:

Clang 19:

-------------------------------------------------------------
Benchmark                   Time             CPU   Iterations
-------------------------------------------------------------
printf                    194 ns          194 ns      3608266
ostream                   446 ns          446 ns      1568557
ostream_fmt_format        187 ns          187 ns      3742475
ostream_std_format        222 ns          222 ns      3151109
fmt_print                 140 ns          139 ns      5025414
std_print                1317 ns         1317 ns       529689
fmt_print_cout            160 ns          160 ns      4382889
std_print_cout           1528 ns         1528 ns       460969

Apple Clang accidentally makes __is_terminal return false directly in its configuration (it does not define _LIBCPP_HAS_NO_VENDOR_AVAILABILITY_ANNOTATIONS), resulting in much faster performance of std::print:

-------------------------------------------------------------
Benchmark                   Time             CPU   Iterations
-------------------------------------------------------------
printf                    195 ns          195 ns      3579025
ostream                   450 ns          450 ns      1555863
ostream_fmt_format        188 ns          188 ns      3721603
ostream_std_format        227 ns          227 ns      3092993
fmt_print                 140 ns          140 ns      5025126
std_print                 205 ns          205 ns      3418570
fmt_print_cout            161 ns          161 ns      4357353
std_print_cout            261 ns          260 ns      2690094

@mordante
Copy link
Member

I have recently benchmarked std::print and fmt::print, and found that the call to isatty can slow down std::print by 6x.

I am aware this is a big slowdown. On my Linux system it's about 2x instead of your 6x. Let me give a short status update.

I still believe the code was implemented correctly and the call to isatty and flush were required by the Standard.
https://cplusplus.github.io/LWG/issue4044 clarified that was not the design intent.
Currently I'm working on other papers that conflict with fixing this P3107R5 has a PR #130500.
There will another PR to implement P3235R3.
Once these patches are merged I want to implement LWG4044, which fixes this issue

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
format C++20 std::format or std::print, and anything related to them libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants