This is an archive of the discontinued LLVM Phabricator instance.

[libc++][format] Granularize formatter_output.
ClosedPublic

Authored by Mordante on Jul 1 2023, 12:01 PM.

Details

Reviewers
hans
ldionne
Group Reviewers
Restricted Project
Commits
rG3ab20c6809d4: [libc++][format] Granularize formatter_output.
Summary

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

Diff Detail

Event Timeline

Mordante created this revision.Jul 1 2023, 12:01 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 1 2023, 12:01 PM
Herald added a subscriber: arichardson. · View Herald Transcript
Mordante updated this revision to Diff 536584.Jul 2 2023, 5:36 AM

CI fixes.

Mordante updated this revision to Diff 538176.Jul 7 2023, 9:39 AM

CI fixes.

Mordante updated this revision to Diff 538341.Jul 8 2023, 2:54 AM

CI fixes.

Mordante published this revision for review.Jul 8 2023, 5:23 AM
Mordante added reviewers: hans, ldionne.
Herald added a project: Restricted Project. · View Herald TranscriptJul 8 2023, 5:23 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript

@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.

@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.

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.

@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.

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.

I doubt the other implementers have noticed this; neither MSVC STL nor libstdc++ have implemented P2286, which introduced this dependency.

ldionne accepted this revision.Jul 10 2023, 9:42 AM

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

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.

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.)

This revision was automatically updated to reflect the committed changes.

@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.

hans added a comment.Jul 11 2023, 6:18 AM

@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.

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.

@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.

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.

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.

@hans I discussed this with @ldionne today. We have some ideas how we can improve this for other new includes in headers too. This is a bit of work and it won't be done before the release. AFAIK you follow HEAD; right? Once I have an improvement I will let you know.