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

Fix more code bloat issues in <format> #1874

Merged
merged 3 commits into from
May 1, 2021
Merged

Fix more code bloat issues in <format> #1874

merged 3 commits into from
May 1, 2021

Conversation

vitaut
Copy link
Contributor

@vitaut vitaut commented Apr 22, 2021

#1851 works around some code bloat issues by "type erasing" the output iterator. This diff fixes the root cause, vformat_to, removing the remaining code bloat issue. It also cleans up format_to and vformat by removing explicit buffer handling from there. format_to_n and formatted_size continue to use buffers directly as an optimization.

This change applies the second fix from P2216 that has been approved by LEWG and LWG and is awaiting plenary. The main change is to remove unnecessary template parameterization of format arguments passed to vformat_to.

Fixes #1883.

@vitaut vitaut requested a review from a team as a code owner April 22, 2021 22:42
@vitaut
Copy link
Contributor Author

vitaut commented Apr 22, 2021

Please note that all but the last commit are the same as in #1851 since I couldn't find a way to make a PR relative to another unmerged one.

@barcharcraz
Copy link
Member

this is the correct way to make the PR relative. You'll have to rebase it when #1851 is merged.

stl/inc/format Show resolved Hide resolved
stl/inc/format Outdated Show resolved Hide resolved
stl/inc/format Outdated Show resolved Hide resolved
@barcharcraz
Copy link
Member

I'd like to benchmark if the array cache type iterator buffer actually helps anything.

@vitaut
Copy link
Contributor Author

vitaut commented Apr 22, 2021

I'd like to benchmark if the array cache type iterator buffer actually helps anything.

I don't think it helps anything right now, it just opens future optimization opportunities that require more work on the lower (writing) level.

@StephanTLavavej
Copy link
Member

Also, don't worry about the force-push when rebasing, this is one of the situations where it's reasonable.

@StephanTLavavej StephanTLavavej added format C++20/23 format performance Must go faster labels Apr 23, 2021
@vitaut vitaut requested a review from barcharcraz April 23, 2021 03:26
@vitaut
Copy link
Contributor Author

vitaut commented Apr 23, 2021

Rebased, switched to _Lazy_locale and removed more unnecessary templatization.

stl/inc/format Outdated Show resolved Hide resolved
stl/inc/format Outdated Show resolved Hide resolved
@StephanTLavavej
Copy link
Member

Ordinarily I'd push changes to address the trivial issues that I found, but then I remembered that #1882 is relative to this PR so I'll be cautious about pushing changes without asking first. I'll go ahead and "approve with suggestions".

@vitaut
Copy link
Contributor Author

vitaut commented Apr 28, 2021

Addressed comments.

stl/inc/format Outdated
return _Handler._Ctx.out();
_OutputIt vformat_to(_OutputIt _Out, const string_view _Fmt, const format_args _Args) {
if constexpr (is_same_v<_OutputIt, _Fmt_it>) {
_Format_handler<char> _Handler(_Out, _Fmt, _Args, _Lazy_locale{});
Copy link
Member

Choose a reason for hiding this comment

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

note (no action required): We know _Fmt_it isn't move only, so we don't have to deal with moving it around.

stl/inc/format Outdated Show resolved Hide resolved
stl/inc/format Outdated Show resolved Hide resolved
stl/inc/format Outdated Show resolved Hide resolved
stl/inc/format Outdated Show resolved Hide resolved
stl/inc/format Show resolved Hide resolved
@barcharcraz
Copy link
Member

Very minor suggestions and then it looks good to go.

We'll probably merge this bound for Dev17, which means we will ship a version of format without it. It's not a problem though because as far as I can tell it's not an ABI break.

@vitaut vitaut requested a review from barcharcraz April 29, 2021 13:34
@StephanTLavavej StephanTLavavej self-assigned this Apr 30, 2021
@StephanTLavavej StephanTLavavej merged commit 02ae70a into microsoft:main May 1, 2021
@StephanTLavavej
Copy link
Member

Thanks for fixing this bloated codegen! This will ship in VS 2022 17.0 Preview 2. 🚀 😻 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
format C++20/23 format performance Must go faster
Projects
None yet
Development

Successfully merging this pull request may close these issues.

<format>: More code bloat when using different iterator types
3 participants