Compile speedup by speth · Pull Request #979 · Cantera/cantera

speth · 2021-02-18T16:05:54Z

The main goal of this PR is to reduce compile times, which have been creeping upward. It does not address the single biggest single build step, compiling the Cython-generated _cantera.cpp file, which is a project unto itself.

Changes proposed in this pull request

Stop using fmt in header-only mode, which speeds up the compilation of each compilation unit that includes fmt
Combine most of the source files defining functions in the VCS_SOLVE class. The whole class is a bit too long for one source file, but 2 compilation units is better than 12.
Use forward declarations to devolve some includes so that fewer header files are linked to each other. In addition offering a small speedup for a full rebuild, this helps limit the number of source files that have to be rebuilt after a change to any header file.
Clean up the constructors for ThermoPhase derivatives. Deprecate a couple of cases which take extra arguments that should be provided using the normal methods for setting parameters (function calls or input files), and eliminate the need for a separate default constructor.
Stop using and deprecate the confusing thermo_t typedef
Drop CI builds using Python 3.5, which is now end of life
Disable debug symbols in CI builds
Tweak the "coverage" build to run a little faster by adding the NDEBUG preprocessor define

Results

For the builds using GitHub Actions,

The standard Ubuntu builds are ~40% faster
The macOS builds are ~16% faster
The "docs" build is ~56% faster
The runs of the examples are ~16% faster
The Windows builds are ~13% faster, though this is somewhat variable
The coverage build is ~15% faster

Checklist

There is a clear use-case for this code change
The commit message has a short title & references relevant issues
Build passes (scons build & scons test) and unit tests address code coverage
The pull request is ready for review

ischoegl · 2021-02-18T16:39:42Z

👍 for getting rid of thermo_t

include/cantera/thermo/BinarySolutionTabulatedThermo.h

bryanwweber · 2021-02-18T19:22:12Z

Stop using fmt in header-only mode, which speeds up the compilation of each compilation unit that includes fmt

The upshot of the header-only mode is that we don't have to add a runtime dependency on fmt. Is it feasible to let this be a configurable option? I think for compiled packages/installers (which I guess would just include the Conda libcantera package and the Ubuntu package?), it would be an advantage to use the header only mode because the compilation time is less important.

As a counterpoint, there is an overhead associated with maintaining the separate option. I'm not sure which is better/worse!

speth · 2021-02-18T19:30:14Z

Stop using fmt in header-only mode, which speeds up the compilation of each compilation unit that includes fmt

The upshot of the header-only mode is that we don't have to add a runtime dependency on fmt. Is it feasible to let this be a configurable option? I think for compiled packages/installers (which I guess would just include the Conda libcantera package and the Ubuntu package?), it would be an advantage to use the header only mode because the compilation time is less important.

As a counterpoint, there is an overhead associated with maintaining the separate option. I'm not sure which is better/worse!

If you compile using the git submodule, the fmt library code still ends up embedded in the Cantera library, so it doesn't add a runtime dependency unless you build using an external copy of the fmt library.

For the Ubuntu package, where we don't (and shouldn't) use our submodules, having a dependency on fmt is pretty simple, and I don't think something we should shy away from. I'd argue the same is true for Conda, where fmt is available in the main conda channel.

bryanwweber · 2021-02-18T21:25:19Z

If you compile using the git submodule, the fmt library code still ends up embedded in the Cantera library

Even for the shared library? I guess the answer is yes, since there's not a libsundials.so file in the build directory. In that case, the change seems fine to me! We'll need to update the conda recipes in parallel with this change. In fact, I wonder if it'd be better to rely on Conda for more of the dependencies...

codecov · 2021-02-18T21:31:46Z

Codecov Report

Merging #979 (cd0b133) into main (9d6095a) will increase coverage by 0.00%.
The diff coverage is 66.84%.

@@           Coverage Diff           @@
##             main     #979   +/-   ##
=======================================
  Coverage   71.56%   71.56%           
=======================================
  Files         370      359   -11     
  Lines       45743    45666   -77     
=======================================
- Hits        32735    32683   -52     
+ Misses      13008    12983   -25

Impacted Files	Coverage Δ
include/cantera/base/AnyMap.h	`100.00% <ø> (ø)`
include/cantera/base/Array.h	`84.00% <ø> (-1.42%)`	⬇️
include/cantera/base/FactoryBase.h	`79.31% <ø> (ø)`
include/cantera/base/utilities.h	`88.88% <ø> (ø)`
include/cantera/base/xml.h	`100.00% <ø> (ø)`
include/cantera/equil/ChemEquil.h	`100.00% <ø> (ø)`
include/cantera/equil/MultiPhase.h	`100.00% <ø> (+3.44%)`	⬆️
include/cantera/kinetics/BulkKinetics.h	`100.00% <ø> (ø)`
include/cantera/kinetics/Falloff.h	`75.00% <ø> (ø)`
include/cantera/kinetics/FalloffMgr.h	`100.00% <ø> (ø)`
... and 185 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9d6095a...f9f2d5d. Read the comment docs.

speth · 2021-02-18T21:47:33Z

If you compile using the git submodule, the fmt library code still ends up embedded in the Cantera library

Even for the shared library? I guess the answer is yes, since there's not a libsundials.so file in the build directory. In that case, the change seems fine to me!

Yes, even for the shared library. We bundle everything that we compile into one library file. For things that we don't compile (specified by system_blah=y), then we link externally.

We'll need to update the conda recipes in parallel with this change. In fact, I wonder if it'd be better to rely on Conda for more of the dependencies...

I'd agree that it's probably a good idea, but I don't see why we have to. Doesn't the conda recipe currently use the submodule for fmt and yaml-cpp? If so, I don't think anything needs to change immediately.

While using the fmt library in header-only mode was convenient, Cantera uses this library in so many separate source files that using it in this mode was noticably increasing overall compilation time.

bryanwweber

Some minor formatting suggestions/questions. Looks good otherwise, refactoring is nice. Thank you for doing this!

bryanwweber · 2021-02-19T22:05:08Z

include/cantera/transport/MixTransport.h

-    //! Calculate the pressure from the ideal gas law
-    doublereal pressure_ig() const {
-        return (m_thermo->molarDensity() * GasConstant *
-                m_thermo->temperature());
-    }
-


Just double checking that removing this entirely is the intention, since you created a new function in MultiTransport.h.

Yes, in the MixTransport class, it was a protected member of that was not actually used.

In the case of MultiTransport, the method is used, so I moved the definition to the .cpp file because doing so eliminates the need to have the full definition of ThermoPhase available in MultiTransport.h.

include/cantera/zeroD/ReactorNet.h

bryanwweber · 2021-02-19T22:17:37Z

src/kinetics/Kinetics.cpp

 }

-void Kinetics::selectPhase(const doublereal* data, const thermo_t* phase,
+void Kinetics::selectPhase(const doublereal* data, const ThermoPhase* phase,


A chance here to change this to double* instead of doublereal*.

I took the opportunity to update this in a few other places as well.

src/zeroD/FlowReactor.cpp

bryanwweber · 2021-02-19T22:28:53Z

One other comment, can you add a "why/how it works briefly" to the commit messages for a045740 and 88ea0f3? I think that'll help if anyone goes to look why those arguments are there.

Code for the VCS_SOLVE class, which was split up among 12 source files, is now consolidated into just vcs_solve.cpp and vcs_solve_TP.cpp. This reduces the amount of time spent compiling the VCS solver by ~50%.

For classes derived from ThermoPhase, instead of having separate constructors for the default constructor and the constructor which takes an input file and phase id, we can use the case where both arguments are the default (empty string) to construct a phase without using an input file. This eliminates the need to repeat any initialization that takes place in the constructor.

Keeping debug symbols around results in much larger object files, which in turn get copied into the various libraries and test executables, and the extra I/O from reading and writing all of these larger files slows down the build.

Compiling with the NDEBUG macro defined skips assertion checks, such as those in standard library code. Skipping these checks allows the tests to run slightly faster. Compiling with a system copy of Sundials speeds up running the test suite slightly because the Sundials library being used is still compiled with optimizations turned on, as opposed to the code within Cantera, that has to be compiled without optimizations so that the line coverage will be accurate.

Python 3.5 reached end of life in September 2020.

speth force-pushed the compile-speedup branch 2 times, most recently from 419bd2d to 8edb731 Compare February 18, 2021 16:28

bryanwweber reviewed Feb 18, 2021

View reviewed changes

include/cantera/thermo/BinarySolutionTabulatedThermo.h Outdated Show resolved Hide resolved

speth force-pushed the compile-speedup branch 2 times, most recently from e4f08e8 to e770385 Compare February 18, 2021 20:36

speth marked this pull request as ready for review February 18, 2021 21:49

speth added 2 commits February 19, 2021 12:42

[SCons] Fix cases where external libraries weren't being linked

72794cf

[SCons] Stop using header-only version of fmt library

17d0fec

While using the fmt library in header-only mode was convenient, Cantera uses this library in so many separate source files that using it in this mode was noticably increasing overall compilation time.

speth force-pushed the compile-speedup branch from 6c1c7d0 to c5be870 Compare February 19, 2021 17:59

speth requested a review from bryanwweber February 19, 2021 18:59

bryanwweber requested changes Feb 19, 2021

View reviewed changes

speth added 12 commits February 19, 2021 18:33

Consolidate VCS solver source files

3babd6b

Code for the VCS_SOLVE class, which was split up among 12 source files, is now consolidated into just vcs_solve.cpp and vcs_solve_TP.cpp. This reduces the amount of time spent compiling the VCS solver by ~50%.

Remove excess includes from Phase.h and ThermoPhase.h

07b9d43

[Kinetics] Remove unused member variable from FalloffMgr

b6bf529

Deprecate thermo_t typedef for class ThermoPhase

486ac3b

Move parts of Array2D implementation out of header

8e7252d

Remove excess includes from header files

dcbd5de

[Thermo] Deprecate m_formGC constructor arg to IdealSolidSolnPhase

a28d06a

Deprecate site density arg to EdgePhase/SurfPhase constructor

20aaf23

[CI] Disable debug symbols in most builds

6384921

Keeping debug symbols around results in much larger object files, which in turn get copied into the various libraries and test executables, and the extra I/O from reading and writing all of these larger files slows down the build.

[CI] Drop Python 3.5 builds

d375bd0

Python 3.5 reached end of life in September 2020.

[CI] Show Ubuntu version in build name

f9f2d5d

speth force-pushed the compile-speedup branch from c5be870 to f9f2d5d Compare February 19, 2021 23:34

bryanwweber approved these changes Feb 21, 2021

View reviewed changes

bryanwweber merged commit d557e50 into Cantera:main Feb 21, 2021

speth deleted the compile-speedup branch February 21, 2021 22:38

Uh oh!

Conversation

speth commented Feb 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ischoegl commented Feb 18, 2021

Uh oh!

Uh oh!

bryanwweber commented Feb 18, 2021

Uh oh!

speth commented Feb 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bryanwweber commented Feb 18, 2021

Uh oh!

codecov bot commented Feb 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

speth commented Feb 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bryanwweber left a comment

Choose a reason for hiding this comment

Uh oh!

bryanwweber Feb 19, 2021

Choose a reason for hiding this comment

Uh oh!

speth Feb 19, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bryanwweber Feb 19, 2021

Choose a reason for hiding this comment

Uh oh!

speth Feb 19, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bryanwweber commented Feb 19, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

speth commented Feb 18, 2021 •

edited

Loading

speth commented Feb 18, 2021 •

edited

Loading

codecov bot commented Feb 18, 2021 •

edited

Loading

speth commented Feb 18, 2021 •

edited

Loading