Use "Include What You Use" on WarpX

[lucafedeli88]

In this rather large PR I have used the tool "Include What You Use" ( iwyu, https://github.com/include-what-you-use/include-what-you-use ) to individuate missing or unnecessary #include directives and possible forward declaration opportunities.

Specifically, I have followed these policies:

• No forward declarations of AMReX classes, except in WarpX.H, since this header is included almost everywhere. Use the new forward declaration headers available in AMReX since PR AMReX/PICSAR: Weekly Update #2016 .
• If a header is already included in MyClass.H, don't include it in MyClass.cpp, even if iwyu suggests to do so.
• Put forward declarations of WarpX classes into _fwd.H files, according to @ax3l suggestion.
• Sometimes I did not included AMReX headers which seemed (to me, at least) not meant to be included directly

Even if I have added many more #include directories than those that I have removed, this PR seems to reduce the compilation time of WarpX in CI. To further reduce the compilation time we may consider to add forward declarations of AMReX classes. (iwyu shows that there are a lot of opportunities to seize) (this has been done).

To use iwyu I just call ccmake from the build directory as follows:

CXX=clang++ CC=clang ccmake  -DCMAKE_CXX_INCLUDE_WHAT_YOU_USE="iwyu" ../.

Then, I save the output of make in a log file (it takes a long time!):

make -j8 > log 2>log

And finally I have a look at this rather huge log file, whose entries are like the following:

/home/luca/Projects/warpx_directory/WarpX/Source/BoundaryConditions/WarpX_PEC.H should remove these lines:

The full include-list for /home/luca/Projects/warpx_directory/WarpX/Source/BoundaryConditions/WarpX_PEC.H:
#include <AMReX_Array.H>                    // for GpuArray
#include <AMReX_Array4.H>                   // for Array4
#include <AMReX_Config.H>                   // for AMREX_SPACEDIM
#include <AMReX_Extension.H>                // for AMREX_FORCE_INLINE
#include <AMReX_GpuQualifiers.H>            // for AMREX_GPU_DEVICE
#include <AMReX_IntVect.H>                  // for IntVect
#include <AMReX_REAL.H>                     // for operator""_rt, Real
#include <AMReX_ccse-mpi.H>                 // for amrex
#include <array>                            // for array
#include <memory>                           // for unique_ptr
#include "Utils/WarpXAlgorithmSelection.H"  // for FieldBoundaryType, FieldB...
#include "WarpX.H"                          // for PatchType
namespace amrex { class MultiFab; }
---

/home/luca/Projects/warpx_directory/WarpX/Source/BoundaryConditions/WarpX_PEC.cpp should add these lines:
#include <AMReX_Box.H>               // for Box
#include <AMReX_Geometry.H>          // for Geometry
#include <AMReX_GpuControl.H>        // for notInLaunchRegion
#include <AMReX_GpuLaunchFunctsC.H>  // for ParallelFor
#include <AMReX_IndexType.H>         // for IndexType
#include <AMReX_MFIter.H>            // for MFIter, TilingIfNotGPU
#include <AMReX_SPACE.H>             // for AMREX_D_DECL

/home/luca/Projects/warpx_directory/WarpX/Source/BoundaryConditions/WarpX_PEC.cpp should remove these lines:
- #include <AMReX.H>  // lines 3-3

The full include-list for /home/luca/Projects/warpx_directory/WarpX/Source/BoundaryConditions/WarpX_PEC.cpp:
#include "BoundaryConditions/WarpX_PEC.H"
#include <AMReX_Box.H>               // for Box
#include <AMReX_Geometry.H>          // for Geometry
#include <AMReX_GpuControl.H>        // for notInLaunchRegion
#include <AMReX_GpuLaunchFunctsC.H>  // for ParallelFor
#include <AMReX_IndexType.H>         // for IndexType
#include <AMReX_MFIter.H>            // for MFIter, TilingIfNotGPU
#include <AMReX_MultiFab.H>          // for MultiFab
#include <AMReX_SPACE.H>             // for AMREX_D_DECL
#include <AMReX_Vector.H>            // for Vector
#include "WarpX.H"                   // for WarpX, PatchType, WarpX::field_b...
---

I used iwyu for a specific configuration (3D, MPI+OMP, QED, PSATD, EB, OPENMP all enabled). Then, I tried to fix all the bugs shown by CI tests.

In a given header file MyClass.H, the include order is the following:

//If there is an associated forward declaration file:
#include "MyClass_fwd.H"

// WarpX headers in alphabetic order
#include "WarpXHeaderA.H"
#include "WarpXHeaderZ.H"

// WarpX forward headers in alphabetic order
#include "WarpXHeaderAA_fwd.H"
#include "WarpXHeaderZZ_fwd.H"

// AMReX headers in alphabetic order
#include <AMReXHeaderA.H>
#include <AMReXHeaderZ.H>

// AMReX forward headers in alphabetic order
#include <AMReXHeaderAA_fwd.H>
#include <AMReXHeaderZZ_fwd.H>

// External libraries headers in alphabetic order
#include <OtherLibraryA.H>
#include <OtherLibraryZ.H>

// STL libraries in alphabetic order
#include <algorithm>
#include <vector>

In the corresponding MyClass.cpp file the only difference is that I MyClass.H at the top.

Warning : depends on AMReX-Codes/amrex@d7ff203

[ax3l]

Cool! Can you please document the usage for IWYU again that did the code transformation? :)

[lucafedeli88]

Cool! Can you please document the usage for IWYU again that did the code transformation? :)

Ehm... I am transforming the code file by file, deciding which of the changes proposed by IWYU is to be accepted! IWYU sometimes proposes files which were never meant to be included directly. Or in some cases it suggests forward declarations. I don't think that we want many forward declarations, but maybe we could consider them in some places...

[lucafedeli88]

I'm not done and there are still some bugs to fix, but the compilation seems a bit faster (maybe ~ 10%).

Actually, most headers need additional includes if we want to follow the "include what you use principle".

I think that the decrease of the compilation time is due to some forward declarations that I have added in WarpX.H.
This header is included many times in the code and has a ton of dependencies... However, I don't know if we really want
forward declarations.

[ax3l]

Forward declarations could be written in explicit Class.def files (which is a common convention many codes use).
Yes, they safe compile time for the case where we just need to know a type exists but don't access members of it, which can potentially save a ton of includes in a translation unit (read as: a .cpp file that does not need to know all details of a full header).

I like .def files because that way, if someone changes the actual header in the .H by e.g. renaming the class such as adding/changing templates, it is clear where the corresponding forward definition needs to be updated (and other files just include the .def instead of writing the same forward definition multiple times).

When this PR is further advanced, we should present the technique in the dev meeting, so others are aware.

[lucafedeli88]

It seems to work at last!

[ax3l]

Uhoh, the docs introduced an issue and now it conflicts :)

Feel free to ping me on Slack once finished and we merge it in.

[lucafedeli88]

@ax3l , it should be OK now!

[ax3l]

Awesome, thanks a lot!
I'll push the last fixes directly.

[ax3l]

I think this file and a.out are accidentally committed.

[ax3l]

now removed :)

[lucafedeli88]

ah yes! thanks! sorry...

[ax3l]

🚀

[ax3l]

Regression addressed in #2044 :)