<print>: Optimize no-argument print() brace handling - #6465
Cornea Cristian (cristi1990an) wants to merge 9 commits into
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
This comment has been minimized.
This comment has been minimized.
We are. That's why _Find_escape_braces returns the index of where the scan short circuited. Escape braces then copies the characters in the preliminary scan directly |
|
Another bothering issue I've noticed and I'll investigate in the scope of this PR is the performance cost of calling println instead of print with '\n' as part of the format string. Early benchmarks don't look great:
|
|
Bettter
|
b868c33 to
a2ad1a5
Compare
|
In my opinion, the optimization in this patch is not worthwhile. For maintainability and consistency, allocating memory here is reasonable. In addition, your patch also introduces new string allocations, which defeats the purpose of the patch. When I previously used GPT to write my character processing program, I also tried something similar, but in the end I gave up on them. AI always tends to assume that the input is mostly ASCII, or to scan for the certain special characters to optimize performance, but they do not help much with performance and greatly complicate the code. |
The point about complexity is fair, in the sense that enabling the optimizations did require far more rewrites than I've originally imagined. From a bird's eye overview I still consider that the complexity isn't necessarily worse or less manageable compared to the original solution. As a summary of the bulkier changes:
One thing to clarify, can you pin point the exact scenario in which we would do additional allocations? This should not be the case. We're only cutting possible allocations |
|
You added string _Output_str in _Print_noformat_nonunicode in <ostream>, and I pointed this out in the review https://github.com/microsoft/STL/pull/6465/changes#r4113171834. It seems you are not familiar with the patch. |
Initially I would've said that this allocation is me just bringing back a string creation that I've removed higher up in _Print_impl because I was tracking the scenario where _Print_noformat_nonunicode is always called with _Add_newline::_Nope. But under further inspection it seems that we do genuinely have a second allocation taking place if _Unescape_braces was called. So this code println(custom_os, "long string.,.. {{}}") first allocates for escaping braces and then allocates one more time for adding the new line. That second allocation only happens on the non-unicode fallback, if cout is detected as a UTF-8 console, it bypasses _Print_noformat_nonunicode and _Output_str. I fixed it in the new commit. I now pass _Add_nl to _Unescape_braces so it adds the new line, then pass _Add_newline::_Nope further down. println(custom_os, "long string...") still needs to allocate because of streambuf design. I would like input on whether any solution for this case can be implemented. |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
<print>: Optimize no-argument print() brace handling
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Copilot review overview
Review effort: Lite
Findings: 3
Open (5)
_Add_nl == _Yes, this implementation allocates and copies the entire payload just to append… · New_Add_nl == _Yes, this implementation allocates and copies the entire payload just to append… · Newostream/filebufwithout an… · Newresize_and_overwriteprovides the requested buffer size as the second lambda parameter, but the… · New
| #ifdef _CPPRTTI | ||
| template <int = 0> | ||
| ios_base::iostate _Print_noformat_nonunicode(ostream& _Ostr, const string_view _Str) { | ||
| ios_base::iostate _Print_noformat_nonunicode(const _Add_newline _Add_nl, ostream& _Ostr, const string_view _Str) { |
There was a problem hiding this comment.
I will need some input on this scenario before I tackle this. Original bot comment found this as an issue and I believe it was right. If not, I'm happy to live with this compromise and sacrifice an allocation for this case.
Appending the newline with a separate sputc changes observable streambuf behavior from the previous single sputn of the complete formatted result. For example, a custom buffer that accepts writes via xsputn but whose overflow returns EOF previously accepts println(os, "text"); this path now writes only text, sets badbit, and loses the newline. Preserve a single contiguous write for the non-console ostream newline path.
There was a problem hiding this comment.
To clarify, it's not clear to me if two consecutive calls to sputn would be allowed here
| string _Output_str; | ||
| string_view _Output = _Str; | ||
| if (_Add_nl == _Add_newline::_Yes) { | ||
| const size_t _Output_size = _Str.size() + 1; | ||
| _Output_str.resize_and_overwrite(_Output_size, [_Str, _Output_size](char* const _Dest_ptr, const size_t) { | ||
| if (!_Str.empty()) { | ||
| _STD char_traits<char>::copy(_Dest_ptr, _Str.data(), _Str.size()); | ||
| } | ||
| _Dest_ptr[_Str.size()] = '\n'; | ||
| return _Output_size; | ||
| }); | ||
| _Output = _Output_str; | ||
| } | ||
|
|
||
| _TRY_IO_BEGIN | ||
| const auto _Characters_written = _Ostr.rdbuf()->sputn(_Str.data(), static_cast<streamsize>(_Str.size())); | ||
| const bool _Was_insertion_successful = static_cast<size_t>(_Characters_written) == _Str.size(); | ||
| const auto _Characters_written = _Ostr.rdbuf()->sputn(_Output.data(), static_cast<streamsize>(_Output.size())); | ||
| const bool _Was_insertion_successful = static_cast<size_t>(_Characters_written) == _Output.size(); |
| void test_noformat_console_ostream() { | ||
| if constexpr (_Is_ordinary_literal_encoding_utf8()) { | ||
| constexpr string_view empty_view{}; | ||
| constexpr string_view nonempty_view{"ostream println"}; | ||
| test::win_console test_console{}; | ||
| FILE* const console_file_stream = test_console.get_file_stream(); | ||
| filebuf console_file_buffer{console_file_stream}; | ||
| ostream console_output{&console_file_buffer}; | ||
|
|
||
| print(console_output, empty_view); | ||
| const bool print_set_badbit = console_output.bad(); | ||
| console_output.clear(); | ||
|
|
||
| println(console_output, empty_view); | ||
| const bool println_set_badbit = console_output.bad(); | ||
| console_output.clear(); | ||
|
|
||
| println(console_output, nonempty_view); | ||
| const bool nonempty_println_set_badbit = console_output.bad(); | ||
| console_output.clear(); | ||
|
|
||
| print(console_output, "ostream marker"); | ||
|
|
||
| assert(!print_set_badbit && !println_set_badbit && !nonempty_println_set_badbit | ||
| && test_console.get_console_line(0).empty() && test_console.get_console_line(1) == L"ostream println" | ||
| && test_console.get_console_line(2) == L"ostream marker"); | ||
| } | ||
| } |
There was a problem hiding this comment.
print calls pubsync before writing to the console and filebuf::sync flushes the underlying FILE*. It then writes through WriteConsoleW so the console output is available when the function returns
| _Output_str.resize_and_overwrite(_Output_size, [_Str, _Output_size](char* const _Dest_ptr, const size_t) { | ||
| if (!_Str.empty()) { | ||
| _STD char_traits<char>::copy(_Dest_ptr, _Str.data(), _Str.size()); | ||
| } | ||
| _Dest_ptr[_Str.size()] = '\n'; | ||
| return _Output_size; |
Optimization for std::print calls with no format arguments,
no new linesand no escaped braces. Current code was allocating an intermediary string by calling _Unescape_braces even if the format string contained no such braces. This might seem like a very niche and constrained case but in my opinion it represents the most common real world case of a print call without format arguments. Allocating an unnecessary string for std::print("Hello long string world ... ") is surprising imo.Optimization is simple in theory, a bit trickier to write down in practice... We do an iteration looking for escaped braces in the format string and if we don't find any, we just call _Vprint... with the string_view. An extended optimization is made here such that if we do find an escape brace at the end of a long string, we don't waste the compute of the search, _Find_escape_braces returns the offset of the found "{{" or "}}" and _Unescape_braces signature is extended with _Escape_brace_offset parameter. The substring _Old_str[0, _Escape_brace_offset) is then copied directly to the resulting string since we've guaranteed no "{{" or "}}" were found in this prefix range.
Edit: after gaining a better understand of the msvc stl\stl\src\print.cpp design I realize that the optimization can be extended to println version too, now that the noformat path doesn't allocate indiscriminantly we can expand _Vprint_unicode_noformat_impl and friends to match _Vprint_unicode_impl and pass the new line flag.
Existing tests already offer enough validation as far as I can tell.Added test for introduced edge case discovered by the bot. We create a console-backed std::ostream and print a default-constructed empty string_view. Afterwards we check that print/println leave the stream in a good state, then print a marker and verify it appears on the line after the empty println.
Codex was used for local repo setup, PR guideline summaries, searching for relevant tests and evaluating impact outside the scope of the enhancement. It was also used for local benchmarks but they were not satisfying enough to add in the PR since they were very prone to noise. These were the most reliable numbers (sending output to a buffered NUL stream):
std::print— long, plainstd::print— long, escaped at the endstd::println— no escapes