This is an archive of the discontinued LLVM Phabricator instance.

[libc++][format] Fixes vector<bool> requirements.
ClosedPublic

Authored by Mordante on Apr 30 2023, 4:48 AM.

Details

Reviewers
ldionne
Group Reviewers
Restricted Project
Commits
rG7e6bcb35a811: [libc++][format] Fixes vector<bool> requirements.
Summary

Makes sure the formatter for the vector<bool>::reference is enabled
when only the header <vector> is included. Before this change it
required <vector> and <format> to be included. This violated the
requirements in the Standard.

Fixes: https://llvm.org/PR61314

Diff Detail

Event Timeline

Mordante created this revision.Apr 30 2023, 4:48 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 30 2023, 4:48 AM
Mordante requested review of this revision.Apr 30 2023, 4:48 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 30 2023, 4:48 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
Mordante updated this revision to Diff 518311.Apr 30 2023, 7:40 AM

Rebased and CI fixes.

Mordante updated this revision to Diff 518312.Apr 30 2023, 7:49 AM

CI fixes.

Mordante updated this revision to Diff 518331.Apr 30 2023, 8:30 AM

CI fixes.

ldionne accepted this revision.May 3 2023, 9:00 AM
ldionne added a subscriber: ldionne.

LGTM but I think we should probably remove the header workaround.

libcxx/include/vector
327

Which ones would we need to remove in order not to have this workaround? I think it might be reasonable to take the hit -- usually our stance is that we try not to remove transitive includes just for the sake of it, however when those transitive includes are a blocker to our work, we remove them. So IMO it would be reasonable to remove the transitive includes causing problems here, unless the impact is really too big.

This revision is now accepted and ready to land.May 3 2023, 9:00 AM
Mordante updated this revision to Diff 519147.May 3 2023, 10:11 AM

Rebased and addresses review comment.

Mordante updated this revision to Diff 519501.May 4 2023, 8:03 AM

Rebased and trigger CI.

Mordante updated this revision to Diff 519541.May 4 2023, 9:45 AM

CI fixes.

Mordante updated this revision to Diff 520058.May 6 2023, 2:44 AM

Rebased and trigger CI.

This revision was landed with ongoing or failed builds.May 6 2023, 5:07 AM
This revision was automatically updated to reflect the committed changes.
hans added a subscriber: hans.Jun 28 2023, 5:40 AM

In Chromium we noticed that this almost doubled the preprocessed size of <vector>, from ca 1.6 MB to 3.2 MB. Since it's a widely included header, that results in ca 8 GB (2.5%) of extra code to compile during a full build.

I realize this change is based on a requirement in the standard, but it seems unfortunate that the cost of the format library is spilling over to vector like this. If there's anything that could be done to reduce the impact here, that would be very valuable to us and presumably other libc++ users too.

In Chromium we noticed that this almost doubled the preprocessed size of <vector>, from ca 1.6 MB to 3.2 MB. Since it's a widely included header, that results in ca 8 GB (2.5%) of extra code to compile during a full build.

I realize this change is based on a requirement in the standard, but it seems unfortunate that the cost of the format library is spilling over to vector like this. If there's anything that could be done to reduce the impact here, that would be very valuable to us and presumably other libc++ users too.

Interesting. Am I right to assume most of the new size is due to including <string>? 2.5% is quite a bit. I'm not sure whether there is a good solution. (Unfortunately modules are not really usable yet.)

I have verified and the bool formatter really needs a string. It needs to use truename which is a string
https://en.cppreference.com/w/cpp/locale/numpunct/truefalsename

hans added a comment.Jun 28 2023, 12:02 PM

Interesting. Am I right to assume most of the new size is due to including <string>?

I don't think it's <string>; that's already widely included. I think it's __format/formatter_output.h where most of it gets added.

Part of me thinks maybe <format> could be gated behind a macro like is was while it was experimental, since we're not currently using it in Chromium. But that would only delay the issue until we do want to use it of course.

Interesting. Am I right to assume most of the new size is due to including <string>?

I don't think it's <string>; that's already widely included. I think it's __format/formatter_output.h where most of it gets added.

@Mordante What's the very very minimum amount of stuff we need to implement formatter<bool>? Are we somehow including a lot more than we strictly need? Can we split up some of those format headers to avoid including some of this stuff transitively?

Part of me thinks maybe <format> could be gated behind a macro like is was while it was experimental, since we're not currently using it in Chromium. But that would only delay the issue until we do want to use it of course.

IMO that's just sweeping the issue under the carpet.

Interesting. Am I right to assume most of the new size is due to including <string>?

I don't think it's <string>; that's already widely included. I think it's __format/formatter_output.h where most of it gets added.

Thanks for the info @hans.

@Mordante What's the very very minimum amount of stuff we need to implement formatter<bool>? Are we somehow including a lot more than we strictly need? Can we split up some of those format headers to avoid including some of this stuff transitively?

At the moment we include the minimum set of granularized headers we need. I had a look yesterday and I found some unneeded includes in our granularized format headers, I'll create a patch for that after testing. I think I can slice up formatter_output.h and reduce the number includes that way. I'll look into that later this week.

Part of me thinks maybe <format> could be gated behind a macro like is was while it was experimental, since we're not currently using it in Chromium. But that would only delay the issue until we do want to use it of course.

IMO that's just sweeping the issue under the carpet.

I agree. However I think the performance of compiling vector is important. So I'm not entirely against it as long as users (chromium) explicitly needs to opt out of it, so by default you get the the current behaviour.