This is an archive of the discontinued LLVM Phabricator instance.

[libc++][format][1/6] Reduce binary size.
ClosedPublic

Authored by Mordante on Sep 26 2021, 4:53 AM.

Details

Reviewers
ldionne
vitaut
Group Reviewers
Restricted Project
Commits
rG0e9979affe29: [libc++][format][1/6] Reduce binary size.
Summary

This removes the format_args_t from <format> and adjusts the type of
the format_args for the vformat_to overloads.

The format_context uses a back_insert_iterator<string> therefore the
new output_iterator function uses a string as its temporary storage
buffer. This isn't ideal. The next patches in this series will improve
this. These improvements make it easy to also improve format_to_n and
formatted_size.

This addresses P2216 6. Binary size.
P2216 5. Compile-time checks are not part of this change.

Implements parts of:

  • P2216 std::format improvements

Depends on D103670

Diff Detail

Event Timeline

Mordante created this revision.Sep 26 2021, 4:53 AM
Mordante requested review of this revision.Sep 26 2021, 4:53 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 26 2021, 4:53 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
Mordante updated this revision to Diff 378267.Oct 8 2021, 9:18 AM

The patches below this one had errors with Buildkite applying patches.
Rebase since all dependencies have landed.

Mordante updated this revision to Diff 380165.Oct 16 2021, 3:54 AM

Rebased to test with wchar_t changes.

Now that the format context is almost always format_context, it might be worth adding [[clang::preferred_name(format_context)]] to the basic_format_context template.

vitaut added inline comments.Oct 17 2021, 7:18 AM
libcxx/include/format
501

Is there a good reason to introduce this function as opposed to having this logic directly in __vformat_to?

Now that the format context is almost always format_context, it might be worth adding [[clang::preferred_name(format_context)]] to the basic_format_context template.

Yes we can also add both format_context and wformat_context, this seems the way we do it for string_view.

libcxx/include/format
501

It was easier for testing, but it seems during cleaning up the patch I never moved it back. Will do so.

Mordante updated this revision to Diff 387070.Nov 14 2021, 5:45 AM
Mordante marked an inline comment as done.

Rebased against main.
Addresses review comments.

vitaut added inline comments.Nov 21 2021, 8:54 AM
libcxx/include/format
527

Maybe force inline format_to* / vformat_to* to make debug codegen less horrible (https://godbolt.org/z/Kah4sqY5z) and since they are just trivial wrappers around __vformat_to*?

LGTM provided that the extra copy and buffering is eliminated in the follow-up diffs. Not directly related to this diff but I recommend doing something about the default codegen (see the inline comment).

vitaut added inline comments.Nov 21 2021, 9:10 AM
libcxx/include/format
527

Note that without this code bloat prevention isn't working even with -O2 right now which is quite worrying: https://godbolt.org/z/1dn836jzG.

Mordante updated this revision to Diff 388982.Nov 22 2021, 11:36 AM

Address review comments.

Mordante marked 2 inline comments as done.Nov 22 2021, 11:41 AM

LGTM provided that the extra copy and buffering is eliminated in the follow-up diffs.

Thanks for the review.
The followup patches in this branch should indeed eliminated the extra copying and buffering.

libcxx/include/format
527

I've added force inlines. I still want to look at the size of the generated code at a later point in time. I think it makes more sense to look at after the all papers and LWG issues have been implemented.

ldionne accepted this revision.Nov 30 2021, 1:42 PM

This LGTM but I am a bit concerned by the use of _LIBCPP_ALWAYS_INLINE. Normally, I would expect the compiler to do the right thing.

This seems reasonable to me since we are applying it systematically on functions that are thin wrappers around __vformat_to, which contains the real meat of the implementation and is not marked as _LIBCPP_ALWAYS_INLINE.

This revision is now accepted and ready to land.Nov 30 2021, 1:42 PM
This revision was automatically updated to reflect the committed changes.
Mordante marked an inline comment as done.