This should reduce the size of the transitive includes for the vector header.
Note the header still quite large so the difference may be small.
Depends on D154122
Paths
| Differential D154286
[libc++][format] Granularize formatter_output. ClosedPublic Authored by Mordante on Jul 1 2023, 12:01 PM.
Details
Summary This should reduce the size of the transitive includes for the vector header. Depends on D154122
Diff Detail
Unit TestsFailed Event TimelineComment Actions @hans can you test whether this solves your issue. I expect it does not, but it would be nice to get confirmation. I don't think I can reduce the since of vector further by granularizing. If that is the case we need to consider conditionally including <__format/*.h> from <vector>. The formatter for vector is available in C++23 and later. Comment Actions
In that case I think we should open a LWG issue to at least raise awareness about this issue. That's also a good way to check whether other implementers have noticed a similar problem. Comment Actions
I doubt the other implementers have noticed this; neither MSVC STL nor libstdc++ have implemented P2286, which introduced this dependency. Comment Actions LGTM anyway but it would be important to raise this concern, cause doubling the preprocessed size of <vector> for a formatter that most code will not make use of (in the near future) is a notable problem. This revision is now accepted and ready to land.Jul 10 2023, 9:42 AM Comment Actions
I agree. I even think in the future this formatter is unlikely to get much usage. This is only needed for vector<bool>. vector<int> is a normal range and its formatter is not in <vector>. As mentioned elsewhere I have a paper where I mention this. To be fully compliant <vector> needs to support, among others, formatter<string, char> too. (This is not in libc++ yet.) Closed by commit rG3ab20c6809d4: [libc++][format] Granularize formatter_output. (authored by Mordante). · Explain WhyJul 10 2023, 10:30 AM This revision was automatically updated to reflect the committed changes. Comment Actions @hans can you test whether this solves the chromium issue with vector. I suspect it doesn't but it would be good to get it confirmed. Comment Actions
For D154122 I measure a 8.4 MB decrease, so that one was small as we suspected. For this new change, we see a 780 MB (0.24%) decrease, which is a nice improvement. I'm also not sure anymore if the full increase we saw was actually from D149543. The update we did included several libc++ revisions, and that one seemed like a prime suspect, but other changes may have contributed too. It's a bit tricky to bisect because the builds are slow and we can't build at every revision. Comment Actions
Thanks for the feedback! Is this reduction enough or should we consider only including <__format/*.h> in C++23 and newer? (C++23 is the language version which introduced formatting for vector<bool>). I don't think that solution is pretty, but <vector> is an important header, so I'm not opposed to making the change. @hans I really like to get your feedback since you're testing this on a larger code base.
Revision Contents
Diff 536584 libcxx/include/CMakeLists.txt
libcxx/include/__chrono/formatter.h
libcxx/include/__format/formatter_char.h
libcxx/include/__format/formatter_floating_point.h
libcxx/include/__format/formatter_integral.h
libcxx/include/__format/formatter_output.h
libcxx/include/__format/formatter_string.h
libcxx/include/__format/write_escaped.h
libcxx/include/module.modulemap.in
libcxx/utils/data/ignore_format.txt
|