Depends on D104942.
Diff Detail
Diff Detail
- Repository
- rG LLVM Github Monorepo
Unit Tests
Unit Tests
Event Timeline
| libcxx/include/__functional/greater.h | ||
|---|---|---|
| 52–55 ↗ | (On Diff #354720) | Oh, did this slip through? I was experimenting with something and then abandoned it. Will fix. |
| libcxx/include/type_traits | ||
| 1366–1375 | Seems I did a copy/paste instead of cut/paste. declval already had its own header but was never removed from type_traits, and we're not violating the ODR by declaring it twice in two different places (but diagnostics point to <type_traits> instead of <utility>). | |
Comment Actions
Nice cleanup!
| libcxx/include/__functional/greater.h | ||
|---|---|---|
| 52–55 ↗ | (On Diff #354720) | I noticed the same change at other places too. |
| libcxx/include/__split_buffer | ||
| 6 | Please sort the headers. | |
| libcxx/include/algorithm | ||
| 655 | Can you remove the duplicated includes and sort them. Not part of your change, but would be nice to do it along in this patch, | |
| libcxx/include/mutex | ||
| 200 | D104942 landed __functional/invoke.h, can you use that? If not please update the comment. | |
| libcxx/include/__functional/identity.h | ||
|---|---|---|
| 27 | Same here, this shouldn't be part of this change. | |
| libcxx/include/concepts | ||
| 216 | Same.. those shouldn't be part of this patch (and if you want to do it throughout in a different patch, let's discuss beforehand). | |
| libcxx/include/module.modulemap | ||
| 716 | What happens if you don't export __tuple? | |
| libcxx/include/concepts | ||
|---|---|---|
| 216 | Hmm... I didn't realise I did it to so many things. I owe @Mordante an apology in that case. I'll do an audit before uploading a new revision. | |
| libcxx/include/module.modulemap | ||
| 716 | Bad things. Seriously though, it means that whatever standard stuff pair.h exports which currently lives in __tuple won't be transitively exported. I haven't properly looked into what __tuple owns, but there are a few things, and we'll need to detangle them from tuple's dependencies. Both of those tasks are far lower on my P-list: I'm trying to focus more on improving potential UX for our existing header splices as well as implementing ranges algos. | |
Comment Actions
So we're splitting up the headers (which I don't love, but will hold my tongue for the most part).
The intention is to provide better name hygene for our users. Meaning headers will export _fewer_ names than they do at present.
This is an intentional BREAKING change. Users will have to update their code to include the correct headers before it will compile.
Before we do this we need to:
- Announce and discuss this change, it's rational, and its benefits to *all libc++ vendors*.
- Document the list of names removed from every header. This list should be complete.
- Ensure Clang's error messages tell users _which_ headers to include. Or at least don't tell users to include the wrong (implementation) headers.
Comment Actions
Agreed. Specifically responding to (3): D105439 is the first of a few patches that I'm working on to get Clang to minimise the pain from breakages and prevent users from making certain mistakes in the interim.
Comment Actions
It is indeed a breaking change, albeit one that is easy to remediate for users. But on a large scale, I agree it can be challenging to tackle.
While there is a clear benefit in making the headers less monolithic, minimizing the includes themselves is, as someone who just wants to ship an updated libc++, primarily a pain point. I'm willing to play ball and go through the pain of fixing my direct users (even though that's a huge pain), but if another vendor finds this change too disruptive, I can also get behind that. At the end of the day, the pain is real, but the benefit has a component of "pedantic correctness" to it. It would be a lot easier to make this change if we somehow could make it gradually, but I think the only way to do that would be for users to make sure their code always builds with modules enabled and disabled.
Comment Actions
I'm starting to form a longer-term plan, so it would be best if we held off on merging this (and its children) till after the branch point at the end of the month. I have no idea how long it will take for my ideas to get integrated into Clang (and other relevant LLVM tools, if necessary). @ldionne I think it would also be good for us to not immediately fix anyone, so that we can get deployment experience (across multiple customers) for the features I end up implementing. Then we can publish a guide explaining how users ought to migrate to libc++next.
Comment Actions
There might be a potential workaround for this effort that lets us make progress, while also not throwing the world under the bus. <Windows.h> has a WIN32_LEAN_AND_MEAN macro that users can opt into if they want a slimmer <Windows.h>. We could have a similar macro that users opt into if they want slimmer libc++ headers for the time being. Once we've gotten tooling to a spot where we're happy users have automated remediation, we can then remove our use of this macro, and deprecate _LIBCPP_LEAN_AND_MEAN.
In summary: no one gets broken, anyone who wants slimmer headers gets them, and we eventually get to the "ideal" state. @ldionne, @EricWF, WDYT?
Comment Actions
Part of me wants to say "no, let's not add yet another configuration that'll only make things more complicated", and part of me wants to say "yes, that will allow us to keep making progress on this header minimization without breaking users".
However, since there is very little benefit to minimizing the headers if nobody's going to turn on that LEAN_AND_MEAN flag, I'm not sure. Instead, here's what I think would be best:
- Minimize the includes in all the detail headers, and in the public headers too.
- However, for "backwards compatibility", keep the unnecessary includes in the public headers, but group them and put them at the end of all the #includes:
#include <__utility/whatever.h> #include <__ranges/whatever.h> // Kept for backwards compatibility until we can remove them #include <utility> #include <whatever>
Eventually, when we're ready to make the move, we'll be able to very easily make the jump for everyone by just removing those stray includes from all public headers. We might need to do a few adjustments before we can drop them (because as time passes, we might sometimes start relying on a few transitive includes by mistake here and there), but that should be much easier to do than what we're doing now.
I guess I'm just somewhat reluctant to adding yet another user-visible knob.
| libcxx/include/algorithm | ||
|---|---|---|
| 655 | I would definitely wait for a separate NFC commit, since doing it here will obscure the changes we're making in this patch, which are all precisely #include-related. | |
Comment Actions
Someone from the Chrome toolchain team sent me an email this morning asking if we can trim our headers, since their include graph blames our user-facing headers as a huge build time sink. My news to them about our current stalling left a bad taste in my mouth, but I was inspired by something that they said about D90072 to think creatively, and then remembered WIN32_LEAN_AND_MEAN. Since they reached out to me, this has gone from "I think this is a space to improve" to "there is a customer requesting something in this space". If they like the idea, I think it's worth entertaining, since Chrome takes forever to build. I've asked someone from that team to chime in as well.
- Minimize the includes in all the detail headers, and in the public headers too.
- However, for "backwards compatibility", keep the unnecessary includes in the public headers, but group them and put them at the end of all the #includes:
#include <__utility/whatever.h> #include <__ranges/whatever.h> // Kept for backwards compatibility until we can remove them #include <utility> #include <whatever>Eventually, when we're ready to make the move, we'll be able to very easily make the jump for everyone by just removing those stray includes from all public headers. We might need to do a few adjustments before we can drop them (because as time passes, we might sometimes start relying on a few transitive includes by mistake here and there), but that should be much easier to do than what we're doing now.
If we're vigilant about our modules build (and I think we're starting to automate a check in that space), then this should only happen while we're including user-facing headers.
I guess I'm just somewhat reluctant to adding yet another user-visible knob.
I don't really like it any more than you do, but it's a user-visible knob with a very short life expectancy.
Comment Actions
Someone from the Chrome toolchain team sent me an email this morning asking if we can trim our headers, since their include graph blames our user-facing headers as a huge build time sink.
Compared to all the other stuff in a big project like Chrome, blaming libc++ surprises me. Are they claiming that Chrome builds faster against libstdc++, then? Any chance of getting them to go public with their measurements? And they're blaming specifically the size of the headers (including, I should add, the disk traffic from opening and closing all these little files), not anything about the actual contents/implementation? E.g. I myself have done a blog post titled "37% of HyperRogue's compilation time is due to std::function" (2019-01-06), but that's nothing to do with disk traffic nor compiler-internal data structures, and everything to do with the bloated std::function API/ABI itself.
I don't really like it any more than you do, but it's a user-visible knob with a very short life expectancy.
User-visible features never really have short life expectancies. ISTR someone talking about "Hyrum's Law" on this very Phabricator instance, fairly recently... ;)
Comment Actions
No. Chrome isn't built against libstdc++ (I'm not sure if it's even possible). Granted, libstdc++ is out of scope.
Any chance of getting them to go public with their measurements?
Good question, but I don't have an answer for you.
And they're blaming specifically the size of the headers (including, I should add, the disk traffic from opening and closing all these little files), not anything about the actual contents/implementation? E.g. I myself have done a blog post titled "37% of HyperRogue's compilation time is due to std::function" (2019-01-06), but that's nothing to do with disk traffic nor compiler-internal data structures, and everything to do with the bloated std::function API/ABI itself.
I'll also leave this question to the Chrome toolchain team to answer, but it seems to me that the little files are hardly a blip on the radar, and that a dominating factor is that we're (libc++) unnecessarily transitively including a lot more than we need to be.
I don't really like it any more than you do, but it's a user-visible knob with a very short life expectancy.
User-visible features never really have short life expectancies. ISTR someone talking about "Hyrum's Law" on this very Phabricator instance, fairly recently... ;)
This won't end up being a Hyrumbug. Hyrum's law states that given a sufficiently large user base and enough time, anything that is observable eventually becomes the interface, even if it's intended to be an implementation detail, which will lead to breaks on anything that's public facing. In this case, we'd have a public-facing macro (so it's already exempt), that would be deprecated when it's no longer necessary (so users stop defining it), and even once we remove said deprecation notice, it won't break anyone. It'll just be a benign eyesore in some code.
Comment Actions
However, since there is very little benefit to minimizing the headers if nobody's going to turn on that LEAN_AND_MEAN flag, I'm not sure. Instead, here's what I think would be best:
Someone from the Chrome toolchain team sent me an email this morning asking if we can trim our headers, since their include graph blames our user-facing headers as a huge build time sink. My news to them about our current stalling left a bad taste in my mouth, but I was inspired by something that they said about D90072 to think creatively, and then remembered WIN32_LEAN_AND_MEAN. Since they reached out to me, this has gone from "I think this is a space to improve" to "there is a customer requesting something in this space". If they like the idea, I think it's worth entertaining, since Chrome takes forever to build. I've asked someone from that team to chime in as well.
If there were a LIBCPP_LEAN_AND_MEAN flag which reduced the header sizes meaningfully, we'd certainly use it.
In order to enable it, having Clang diagnostics with good fixit hints would certainly help, but we could probably do without it too with some scripting, so don't block on that on our account.
We're not blaming our slow builds on libc++, but we're looking everywhere we can, and if the libc++ headers could be made slimmer, it would certainly help since we rely on many of them almost everywhere, and I'd expect it would be a win for other projects too.
The numbers we're looking at are public, and can be found here: https://commondatastorage.googleapis.com/chromium-browser-clang/include-analysis.html That doesn't measure compile time, but compiler input size. In other measurements we've seen a strong correlation between compiler input size and compile time. It's not surprising that libc++ headers are on top since they're included everywhere, but that also means there's a high chance slimmer libc++ headers would have measurable impact for us.
Comment Actions
The numbers we're looking at are public, and can be found here: https://commondatastorage.googleapis.com/chromium-browser-clang/include-analysis.html That doesn't measure compile time, but compiler input size. In other measurements we've seen a strong correlation between compiler input size and compile time. It's not surprising that libc++ headers are on top since they're included everywhere, but that also means there's a high chance slimmer libc++ headers would have measurable impact for us.
The same analysis for an LLVM build is available here: https://commondatastorage.googleapis.com/chromium-browser-clang/llvm-include-analysis.html though that uses libstdc++ (and hasn't been updated in a while). The standard headers show up on top, and I assume that's common across most c++ projects.
Same here, this shouldn't be part of this change.