Skip to content
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

Implement P3107R5 optimized <print> #4821

Merged
merged 58 commits into from
Sep 4, 2024

Conversation

blackninja9939
Copy link
Contributor

@blackninja9939 blackninja9939 commented Jul 8, 2024

Fixes #4509

Is my first PR so I expect plenty of fun times and issues 😅

In my benchmarking the non-buffered version is about 400-700ns faster, which is not as much as I would have hoped, but I do not think there is a huge amount of room in the design of format and Window's write API for great gains.

The format API ties the iterator type to everything, so we always must go through _Fmt_iterator_buffer writing char's into the buffer and flushing on demand, so using _fputc_nolock is hard since we never actually do anything character by character for the wrapped iterator and the type is important.
Instead I've generalised the optimization vector and string were doing of a customization point for the wrapped iterator when we flush and used that in these custom iterators to write to the stream or to the console with the parent holding a lock. This avoids need to ever allocate the final std::string or re-take the lock.

If we write to the console we must additionally transcode to wchar_t so we can call WriteConsoleW, I've reduced allocation there as well since it can often fit in a buffer but it is still work being done.

Its still a speedup, and I'm sure there is extra room for gains, but I tried various approaches and this was the "simplest" and reduced the most allocation calls.

@blackninja9939 blackninja9939 requested a review from a team as a code owner July 8, 2024 16:33
@blackninja9939 blackninja9939 force-pushed the mclohessy/P3107 branch 3 times, most recently from 82ef338 to 411e94e Compare July 8, 2024 16:37
@blackninja9939
Copy link
Contributor Author

@blackninja9939 please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.

@microsoft-github-policy-service agree [company="{your company}"]

Options:

  • (default - no company specified) I have sole ownership of intellectual property rights to my Submissions and I am not making Submissions in the course of work for my employer.
@microsoft-github-policy-service agree
  • (when company given) I am making Submissions in the course of work for my employer (or my employer has intellectual property rights in my Submissions by contract or applicable law). I have permission from my employer to make Submissions and enter into this Agreement on behalf of my employer. By signing below, the defined term “You” includes me and my employer.
@microsoft-github-policy-service agree company="Microsoft"

Contributor License Agreement

@microsoft-github-policy-service agree

@blackninja9939 blackninja9939 changed the title Implement P3107R5 Implement P3107R5 optimized <print> Jul 8, 2024
benchmarks/src/print.cpp Outdated Show resolved Hide resolved
stl/inc/print Outdated Show resolved Hide resolved
stl/src/print.cpp Outdated Show resolved Hide resolved
@StephanTLavavej StephanTLavavej added cxx23 C++23 feature defect report Applied retroactively labels Jul 8, 2024
@StephanTLavavej

This comment was marked as resolved.

stl/inc/format Outdated Show resolved Hide resolved
@AlexGuteniev

This comment was marked as resolved.

stl/inc/__msvc_formatter.hpp Outdated Show resolved Hide resolved
stl/inc/print Outdated Show resolved Hide resolved
stl/src/print.cpp Outdated Show resolved Hide resolved
@StephanTLavavej StephanTLavavej self-assigned this Jul 10, 2024
@StephanTLavavej StephanTLavavej changed the title Implement P3107R5 optimized <print> Implement P3107R5 optimized <print> Jul 17, 2024
Copy link
Member

@CaseyCarter CaseyCarter left a comment

Choose a reason for hiding this comment

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

Surprisingly few changes for as big as this is. Very nice work.

stl/inc/yvals_core.h Show resolved Hide resolved
tests/libcxx/expected_results.txt Outdated Show resolved Hide resolved
stl/src/print.cpp Outdated Show resolved Hide resolved
stl/src/print.cpp Outdated Show resolved Hide resolved
stl/src/print.cpp Outdated Show resolved Hide resolved
stl/inc/print Outdated Show resolved Hide resolved
stl/inc/print Outdated Show resolved Hide resolved
stl/inc/print Outdated Show resolved Hide resolved
stl/inc/print Outdated Show resolved Hide resolved
stl/inc/format Outdated Show resolved Hide resolved
@CaseyCarter

This comment was marked as resolved.

I adjusted the specializations at the last minute, but didn't make the corresponding test changes.
@CaseyCarter CaseyCarter removed their assignment Aug 30, 2024
@StephanTLavavej StephanTLavavej self-assigned this Aug 30, 2024
tests/libcxx/expected_results.txt Show resolved Hide resolved
tests/libcxx/expected_results.txt Outdated Show resolved Hide resolved
benchmarks/src/efficient_nonlocking_print.cpp Show resolved Hide resolved
stl/inc/print Outdated Show resolved Hide resolved
stl/inc/print Outdated Show resolved Hide resolved
stl/inc/print Outdated Show resolved Hide resolved
benchmarks/src/efficient_nonlocking_print.cpp Outdated Show resolved Hide resolved
benchmarks/src/efficient_nonlocking_print.cpp Show resolved Hide resolved
@StephanTLavavej StephanTLavavej removed their assignment Aug 30, 2024
@StephanTLavavej
Copy link
Member

Thanks, this is incredible! 😻 I pushed individual commits for the remaining issues I noticed, all small (the absence of const on a test format being the most significant).

We merge PRs to the GitHub and MSVC-internal repos in a semi-manual process, batched up to save time. Your PR will be part of the next batch, likely next week. I'll post comments here as I prepare it for merging!

@StephanTLavavej StephanTLavavej self-assigned this Sep 3, 2024
@StephanTLavavej
Copy link
Member

I'm mirroring this to the MSVC-internal repo - please notify me if any further changes are pushed.

@StephanTLavavej StephanTLavavej merged commit a36ece0 into microsoft:main Sep 4, 2024
39 checks passed
@StephanTLavavej
Copy link
Member

Thanks for implementing this C++23 DR, and congratulations on your first microsoft/STL commit! 😻 🥳 🍰

This change is expected to ship in Preview 1 of whatever comes after VS 2022 17.12 (we don't know what it'll be yet, so we're calling it VS Meow in the Changelog until we do know).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cxx23 C++23 feature defect report Applied retroactively
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

P3107R5 Permit An Efficient Implementation Of <print>
5 participants