[libc++][WIP] Try disabling transitive includes in all configurations to see impact on CI times

[Mordante]

libcxx/utils/libcxx/test/params.py

210

Note this means the transitive includes test will be disabled too.
So when this saves time we need a way to enable that test again.

[ldionne]

Across our test suite, this does seem to provide a pretty significant compilation speed up.

Without transitive includes (this change)

With transitive includes (status quo)

Overall, this seems to provide roughly a 20% improvement in the speed of the CI. It is possible that this would also provide a similar speed up in the compilation of code bases using C++ heavily assuming they include what they use fairly consistently.

[ldionne]

Actually here's a breakdown of the timings and improvement for that CI run:

        Before change (s)   After change (s)    speedup
C++03   242                 224                 8 %
C++11   316                 275                 14.9%
C++14   351                 284                 23.5%
C++17   391                 335                 16.7%
C++20   650                 521                 24.7%

[Mordante]

In D150300#4342980, @ldionne wrote:

Actually here's a breakdown of the timings and improvement for that CI run:

        Before change (s)   After change (s)    speedup
C++03   242                 224                 8 %
C++11   316                 275                 14.9%
C++14   351                 284                 23.5%
C++17   391                 335                 16.7%
C++20   650                 521                 24.7%

This looks very promising!

I think we should adjust transitive_includes.sh.cpp test to make sure it undefines _LIBCPP_REMOVE_TRANSITIVE_INCLUDES and allow it to run with enable_transitive_includes disabled. That should be fairly simple.

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.

[ldionne]

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

[ldionne]

In D150300#4343062, @Mordante wrote:

I think we should adjust transitive_includes.sh.cpp test to make sure it undefines _LIBCPP_REMOVE_TRANSITIVE_INCLUDES and allow it to run with enable_transitive_includes disabled. That should be fairly simple.

I just removed the transitive-includes-disabled feature and it seemed to work out of the box (which is weird).

[philnik]

In D150300#4343616, @ldionne wrote:

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

[EricWF]

Do we have -ftime-trace files for this change?

@ldionne One of your major concerns is that we test as we ship. Is this how we ship?

[EricWF]

In D150300#4344047, @philnik wrote:

In D150300#4343616, @ldionne wrote:

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

[ldionne]

Abandoning since this was a CI experiment and hopefully that isn't needed anymore since the Clang builders are off of the libc++ ones.

libcxx/utils/libcxx/test/params.py

Parameter(name='enable_assertions', choices=[True, False], type=bool, default=False,
             ] if assertions else []),
 
   Parameter(name='additional_features', type=list, default=[],
             help="A comma-delimited list of additional features that will be enabled when running the tests. "
                  "This should be used sparingly since specifying ad-hoc features manually is error-prone and "
                  "brittle in the long run as changes are made to the test suite.",
             actions=lambda features: [AddFeature(f) for f in features]),
 
-  Parameter(name='enable_transitive_includes', choices=[True, False], type=bool, default=True,
+  Parameter(name='enable_transitive_includes', choices=[True, False], type=bool, default=False,
             help="Whether to enable backwards-compatibility transitive includes when running the tests. This "
                  "is provided to ensure that the trimmed-down version of libc++ does not bit-rot in between "
                  "points at which we bulk-remove transitive includes.",
             actions=lambda enabled: [] if enabled else [
               AddFeature('transitive-includes-disabled'),
               AddCompileFlag('-D_LIBCPP_REMOVE_TRANSITIVE_INCLUDES')
             ]),
 ]