1651951 - Warn when a patch makes a header expensive to include

Status: NEW

(Developer Infrastructure :: Source Code Analysis, enhancement, P4)

Description

[Marco Castelluccio [:marco]]

Using something like https://github.com/aras-p/ClangBuildAnalyzer, we could check the list of expensive headers before/after a patch (or a set of patches) and warn when some patch is adding expensive headers (or making some already existing headers way more expensive).

This would help not regressing the improvements made in bug 785103.

Comment 1

[Mike Hommey [:glandium]]

Header expensiveness is a function of the number of times it's included and how much of it is used each time. IOW, to get the picture you want to warn about, you essentially need an entire build. Times two. That seems like a lot of CPU time for a lint.

Comment 2

[Marco Castelluccio [:marco]]

(In reply to Mike Hommey [:glandium] from comment #1)

Header expensiveness is a function of the number of times it's included and how much of it is used each time. IOW, to get the picture you want to warn about, you essentially need an entire build. Times two. That seems like a lot of CPU time for a lint.

Yeah, my idea was to run this periodically (e.g. on mozilla-central pushes) and 1) warn when a push is increasing the expensiveness; 2) warn when a patch is introducing usage of an expensive header.
The lint at review time would just be 2, which doesn't need to run the full analysis but can just use the results from the latest analysis.

For 1, of course we'd need to define some kind of process (that is, who would receive the warning?).

Comment 3

[Mike Hommey [:glandium]]

For 2, though, if a header is expensive, it's usually because it's included > 500 times. One more include is not actually going to hurt.

Comment 4

[Simon Giesecke [:sg] [he/him]]

(In reply to Mike Hommey [:glandium] from comment #3)

For 2, though, if a header is expensive, it's usually because it's included > 500 times. One more include is not actually going to hurt.

I agree, that's not worth identifying in terms of the reviewbot.

Somewhat more interesting would be 3) adding an include to a header that is included lots of times that makes it significantly more expensive to include

Comment 5

[Andi [:andi]]

I think we should look this from the perspective when we will migrate to C++20 and when we are going to use modules, this will bring us advantages such as:

• build time decrease
• TU isolation from macros/defines
• no need for so many header files since the definition doesn't have to be separated from the declaration
• no more include guards in the header files

Comment 6

[Simon Giesecke [:sg] [he/him]]

While it is correct that C++20 modules will bring fundamental changes to this problem, I fear t will take a long time to migrate to C++20 modules, if this is ever done at all. For the meantime, it would be great to have this, as the long build times are a huge impediment for efficient local development, even if they do not bring the major costs on automation compared to test execution.

Comment 7

[Nathan Froyd [:froydnj]]

(In reply to Andi-Bogdan Postelnicu [:andi] from comment #5)

I think we should look this from the perspective when we will migrate to C++20 and when we are going to use modules, this will bring us advantages such as:

It's also worth pointing out, in addition to what Simon said, that modules support is dependent on support in our current and baseline compilers. Given that both clang and GCC have, at best, basic module support (https://gcc.gnu.org/projects/cxx-status.html and http://clang.llvm.org/cxx_status.html are not exactly encouraging on that front, although https://gcc.gnu.org/wiki/cxx-modules suggests the wiki is out of data), it's going to be a very long time before support in the newest compilers works and support is present in the baseline compilers that we use.

Comment 8

[Marco Castelluccio [:marco]]

Interesting: https://github.com/serge-sans-paille/preprocessor-utils/blob/master/iwyu-diff.