Skip to content

Conversation

@AdamBucior
Copy link
Contributor

Related to #1802.

@AdamBucior AdamBucior requested a review from a team as a code owner April 5, 2021 21:04
@AdamBucior

This comment has been minimized.

@StephanTLavavej StephanTLavavej added format C++20/23 format performance Must go faster labels Apr 5, 2021
stl/inc/format Outdated
if constexpr (is_same_v<_ArgTy, basic_string_view<_CharT>>) {
_Result += _Arg.size();
} else if constexpr (is_same_v<_ArgTy, const _CharT*>) {
_Result += char_traits<_CharT>::length(_Arg);
Copy link
Member

Choose a reason for hiding this comment

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

Could this lead to determining the strlen multiple times? Do we cache it anywhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm afraid it could since _Write function is converting it to a basic_string_view later. Since caching it here would be problematic, we could convert all const _CharT* to basic_string_view in make_(w)format_args, although it would probably be non-conformant (but TBH the chance of anyone even noticing it would be extremely low, so I think we should do that (we can also file an LWG issue to allow us for it)).

Copy link
Contributor

Choose a reason for hiding this comment

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

that conversion itself would run strlen, just pick a number

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that conversion itself would run strlen

That's the point, we call strlen once in make_(w)format_args, we don't need to call it twice (once here, and once in _Write)

Copy link
Contributor

@CaseyCarter CaseyCarter Apr 7, 2021

Choose a reason for hiding this comment

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

The point of storing const CharT* as const CharT* instead of basic_string_view is that we can potentially avoid strlen entirely if the format spec specifies a maximum length. We don't want to fill the memory caches with garbage because a user passed a pointer to an array of 3GB of characters, 7 of which they'd like to print.

Since this is an estimation, and we don't want our optimization to be self-defeating, I'd suggest simply picking a number like 16 or 32 as the estimated length of const CharT* arguments.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I might be missing something but it seems to me that _Write function for const CharT* is just calling _Write function for basic_string_view.

STL/stl/inc/format

Lines 1909 to 1913 in c6f02b6

template <class _CharT, class _OutputIt>
_NODISCARD _OutputIt _Write(
_OutputIt _Out, const _CharT* _Value, const _Basic_format_specs<_CharT>& _Specs, locale _Locale) {
return _Write(_Out, basic_string_view<_CharT>{_Value}, _Specs, _Locale);
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also is there a format spec for string that specifies maximum length?

Copy link
Contributor

@CaseyCarter CaseyCarter Apr 7, 2021

Choose a reason for hiding this comment

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

I might be missing something but it seems to me that _Write function for const CharT* is just calling _Write function for basic_string_view.

We're not finished yet =)

Also is there a format spec for string that specifies maximum length?

[format.string.std]/14:

The nonnegative-integer in precision is a decimal integer defining the precision or maximum field size. ... For string types, this field provides an upper bound for the estimated width of the prefix of the input string that is copied into the output.

@miscco
Copy link
Contributor

miscco commented Apr 6, 2021

I like the change, but I regarding @StephanTLavavej concern about multiple calls to strlen I believe we should go the extra mile and inline vformat_to

template <output_iterator<const char&> _OutputIt>
_OutputIt vformat_to(
    _OutputIt _Out, const string_view _Fmt, const format_args_t<type_identity_t<_OutputIt>, char> _Args) {
    _Format_handler<_OutputIt, char, basic_format_context<_OutputIt, char>> _Handler(
        _Out, _Fmt, _Args, locale::classic());
    _Parse_format_string(_Fmt, _Handler);
    return _Handler._Ctx.out();
}

I is only 2-3 lines of code and once we have the _Handler we should be more or less save as that has already out everything into the _Format_arg_store

That said, we can always do that later, so feel free to diregard. 👍 for throwing const char* in the bin and always using a string_view

@barcharcraz
Copy link
Contributor

As this is not ABI breaking and we're working on getting actually merged into master I'd like to delay actually merging this PR for a while.

@StephanTLavavej StephanTLavavej changed the base branch from feature/format to main April 15, 2021 09:47
@StephanTLavavej
Copy link
Member

Because we squash-merged #1821 into main (for delicious linear history), I've force-pushed and retargeted this PR to main. All that I did was cherry-pick this PR's 4 commits onto main, with no merge conflicts needing manual resolution.

Apologies for the force push!

@StephanTLavavej StephanTLavavej self-assigned this Apr 21, 2021
@StephanTLavavej StephanTLavavej merged commit 14eb90e into microsoft:main Apr 22, 2021
@StephanTLavavej
Copy link
Member

Thanks for this significant performance improvement! This should ship in VS 2019 16.10 Preview 3 along with <format> itself (going to Changelog this for 17.0 Preview 2, but then immediately prepare a backport). 🚀 😸 🎉

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.

5 participants