[libcxx-commits] [PATCH] D150300: [libc++][WIP] Try disabling transitive includes in all configurations to see impact on CI times

[Eric Fiselier via Phabricator via libcxx-commits]

EricWF added a comment.

In D150300#4344047 <https://reviews.llvm.org/D150300#4344047>, @philnik wrote:

> In D150300#4343616 <https://reviews.llvm.org/D150300#4343616>, @ldionne wrote:
>
>> In D150300#4343062 <https://reviews.llvm.org/D150300#4343062>, @Mordante wrote:
>>
>>> I also wonder whether we should look at removing the transitive includes in general (obviously in a separate patch). I expected some overhead from allowing them, but not these numbers. I still don't like it might break some users, but the benefits seem quite large.
>>
>> Yeah, that's what this patch got me thinking about, too. This is kind of scary but it would likely provide worthwhile benefits to users. I do think landing a variant of this patch (aka where we change the default in the CI) is tied to removing the transitive includes by default for users, since we need to keep testing exactly what we ship, not something slightly different.
>>
>> So if we were to remove the transitive includes for users, we'd want to give some grace period for users to update their code base, and give them a way to opt-in gradually into that change. So for example, we could:
>>
>> 1. Stop removing transitive includes under `_LIBCPP_REMOVE_TRANSITIVE_INCLUDES`, now when we remove transitive includes we start guarding it behind `_LIBCPP_REMOVE_TRANSITIVE_INCLUDES_2`.
>> 2. Release-note that transitive includes will be removed by default in LLVM 18 (for example), and they can define `_LIBCPP_REMOVE_TRANSITIVE_INCLUDES` in LLVM 17 to start preparing their code base.
>> 3. In LLVM 18, we flip the default to remove transitive includes and optionally we provide a way to opt into the old behavior with a macro like `_LIBCPP_PROVIDE_TRANSITIVE_INCLUDES`. This is the grace period.
>
> IMO it would make more sense to add a new macro, since the users who already define `_LIBCPP_REMOVE_TRANSITIVE_INCLUDES` are fine with the transitive includes not being stable.
>
>> 4. In LLVM 19, we remove the escape hatch and unconditionally remove all transitive includes marked under `_LIBCPP_REMOVE_TRANSITIVE_INCLUDES`.
>> 5. Now we begin the same cycle again but with `_LIBCPP_REMOVE_TRANSITIVE_INCLUDES_2`.
>> 6. Optionally, we add a `#warning` if `_LIBCPP_REMOVE_TRANSITIVE_INCLUDES` is defined just so that people who used that macro to opt-in their code bases earlier can clean up their build scripts.
>>
>> In all cases, we can never reuse the `_LIBCPP_REMOVE_TRANSITIVE_INCLUDES` name unless we know nobody's defining the macro anymore because otherwise we could break them.
>
> Given that we already granularized most of the relevant stuff I wonder whether it's even worth it to start a new cycle instead of just removing includes unconditionally after we've done it once. After that way fewer people will be broken by removing transitive includes. I also can't find many more headers which we would want to granulize. My remaining candidates are `<math.h>`, `<mutex>`, `<locale>` and possibly `<future>`, `<ios>`, `<istream>` and `<regex>`. AFAIK everything else is already granularized, a single-class header or quite small.

Whats' the point of granulazing headers that aren't included elsewhere by libc++? It seems like we couldn't possibly decrease their include surface.
The damage that's done by granularization is sizable and consequential. We should only be doing it if we can justify that cost.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D150300/new/

https://reviews.llvm.org/D150300