-
-
Notifications
You must be signed in to change notification settings - Fork 346
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Compile speedup #979
Compile speedup #979
Conversation
419bd2d
to
8edb731
Compare
👍 for getting rid of |
The upshot of the header-only mode is that we don't have to add a runtime dependency on 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 For the Ubuntu package, where we don't (and shouldn't) use our submodules, having a dependency on |
e4f08e8
to
e770385
Compare
Even for the shared library? I guess the answer is yes, since there's not a |
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
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
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 |
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.
6c1c7d0
to
c5be870
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some minor formatting suggestions/questions. Looks good otherwise, refactoring is nice. Thank you for doing this!
//! Calculate the pressure from the ideal gas law | ||
doublereal pressure_ig() const { | ||
return (m_thermo->molarDensity() * GasConstant * | ||
m_thermo->temperature()); | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just double checking that removing this entirely is the intention, since you created a new function in MultiTransport.h
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
.
src/kinetics/Kinetics.cpp
Outdated
@@ -267,7 +269,7 @@ void Kinetics::checkReactionBalance(const Reaction& R) | |||
} | |||
} | |||
|
|||
void Kinetics::selectPhase(const doublereal* data, const thermo_t* phase, | |||
void Kinetics::selectPhase(const doublereal* data, const ThermoPhase* phase, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A chance here to change this to double*
instead of doublereal*
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I took the opportunity to update this in a few other places as well.
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.
c5be870
to
f9f2d5d
Compare
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
fmt
in header-only mode, which speeds up the compilation of each compilation unit that includesfmt
VCS_SOLVE
class. The whole class is a bit too long for one source file, but 2 compilation units is better than 12.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.thermo_t
typedefNDEBUG
preprocessor defineResults
For the builds using GitHub Actions,
Checklist
scons build
&scons test
) and unit tests address code coverage