Skip to content
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

optimize build time (precompiled headers) #1800

Closed
wants to merge 1 commit into from

Conversation

pcaspers
Copy link
Contributor

@pcaspers pcaspers commented Oct 1, 2023

Playing with pre-compiled headers as an alternative way to speed up the build - these are pretty easy to add to a cmake build and have quite an impact on parse times:

baseline (target ql_library only)

Compilation (1854 times):
  Parsing (frontend):         1270.3 s
  Codegen & opts (backend):    542.4 s

after change

Compilation (1863 times):
  Parsing (frontend):          512.8 s
  Codegen & opts (backend):    539.1 s

Thoughts? Is this a no-brainer or am I overlooking issues introduced by that?

@sweemer
Copy link
Contributor

sweemer commented Oct 1, 2023

Interesting. What if you add ql/quantlib.hpp to the list of precompiled headers - does it transitively apply to all headers on all platforms, or do they all have to be individually specified?

@coveralls
Copy link

Coverage Status

coverage: 71.914%. remained the same when pulling caabd56 on pcaspers:optimize_build_time_pch into eaa249d on lballabio:master.

@xcelerit-team
Copy link

Interesting. What if you add ql/quantlib.hpp to the list of precompiled headers - does it transitively apply to all headers on all platforms, or do they all have to be individually specified?

That's a bad idea for incremental builds - if you do an ever-so-slight change to any header, it'll cause a recompilation of every single translation unit in the whole code base.

It's best to add only headers that hardly ever change. The standard headers and boost ones that are used most often throughout the library are the best candidates.

QuantLib's own headers shouldn't be included unless they are both never really touched (e.g. in months) and included in several translation units (to get a compile time gain). Best is to avoid them completely.

@pcaspers pcaspers marked this pull request as draft October 1, 2023 16:46
@pcaspers
Copy link
Contributor Author

pcaspers commented Oct 1, 2023

Well, I included the headers that are most expensive according to the results from ClangBuildAnalyzer described in #1787. I guess we could replace errors.hpp with sstream with a similar gain. But there are also ql headers that are expensive on their own. I don't think any of those in the list are subject to particularly frequent changes.

@xcelerit-team
Copy link

The comment was more about putting the full quantlib.hpp into the list, which will include everything.

Does the build analyzer allow showing only the third party headers, sorted by time spent + number of includes in total? Usually the boost headers and streams tend to be expensive to parse + they are included a lot.

Regarding infrequent changes to those QuantLib headers: Also consider the indirect includes of these. They should be very stable too.

Another point to be aware of with precompiled headers: Cmake puts an implicit include of all the listed headers into every translation unit. (That's why all files get recompiled if any of these headers are touched.) This can lead to developers forgetting to include files they need explicitly, potentially causing issues for third party users of the library. But I believe there's already a CI/CD workflow guarding against that...

@pcaspers
Copy link
Contributor Author

pcaspers commented Oct 1, 2023

ClangBuildAnalyzer has very few options, I don't know how to filter on external headers.

On your last point. Indeed, I just ran into an issue because incrementalstatistics.hpp includes boost accumulator files and that (once again) triggered boostorg/accumulators#20 in some cpp files from which we also include from boost accumulator. So either we need to remove incrementalstatistics.hpp from the precompiled header list, or we need to refactor the class so that we instantiate the accumulators in incrementalstatistics.cpp. I'd prefer the latter but I'll open a separate issue / PR for that.

@pcaspers
Copy link
Contributor Author

pcaspers commented Oct 2, 2023

There are complications when using ccache though: https://ccache.dev/manual/3.2.4.html#_precompiled_headers

@lballabio
Copy link
Owner

cmake seems to use -include as needed by ccache (see e.g. https://github.com/lballabio/QuantLib/actions/runs/6370437274/job/17291547349?pr=1800), but I don't know about the sloppiness thing.

@pcaspers
Copy link
Contributor Author

pcaspers commented Oct 3, 2023

Ah yes, I observe this in my local builds as well, that's nice. As for the sloppiness I can enable that in our docker build by adding an environment variable

ENV CCACHE_SLOPPINESS="pch_defines,time_macros"

I suppose something similar will be possible in the GitHub workflows?

@lballabio
Copy link
Owner

Yes, it's possible to define env variables, no problem.

I'm somewhat surprised that <ql/math/statistics/incrementalstatistics.hpp> or <ql/math/interpolations/cubicinterpolation.hpp> are included by so many files that they need to go in the pch. Maybe we should look into why.

@pcaspers
Copy link
Contributor Author

Looks like incrementalstatistics.hpp is included 11 times

grep --include="*.?pp" -nH --null -r -e "incrementalstatistics.hpp" * 
ql/experimental/volatility/zabr.hpp�30:#include <ql/math/statistics/incrementalstatistics.hpp>
ql/methods/montecarlo/longstaffschwartzpathpricer.hpp�31:#include <ql/math/statistics/incrementalstatistics.hpp>
ql/math/statistics/incrementalstatistics.hpp�22:/*! \file incrementalstatistics.hpp
ql/math/statistics/all.hpp�9:#include <ql/math/statistics/incrementalstatistics.hpp>
ql/math/statistics/histogram.cpp�21:#include <ql/math/statistics/incrementalstatistics.hpp>
ql/math/statistics/incrementalstatistics.cpp�22:#include <ql/math/statistics/incrementalstatistics.hpp>
ql/math/statistics/sequencestatistics.hpp�28:#include <ql/math/statistics/incrementalstatistics.hpp>
test-suite/hybridhestonhullwhiteprocess.cpp�37:#include <ql/math/statistics/incrementalstatistics.hpp>
test-suite/stats.cpp�26:#include <ql/math/statistics/incrementalstatistics.hpp>
test-suite/riskstats.cpp�24:#include <ql/math/statistics/incrementalstatistics.hpp>
test-suite/americanoption.cpp�33:#include <ql/math/statistics/incrementalstatistics.hpp>

and cubicinterpolation.hpp is included 28 times

grep --include="*.?pp" -nH --null -r -e "cubicinterpolation.hpp" * 
Examples/MulticurveBootstrapping/MulticurveBootstrapping.cpp�49:#include <ql/math/interpolations/cubicinterpolation.hpp>
ql/experimental/volatility/zabr.hpp�32:#include <ql/math/interpolations/cubicinterpolation.hpp>
ql/experimental/inflation/polynomial2Dspline.hpp�30:#include <ql/math/interpolations/cubicinterpolation.hpp>
ql/methods/finitedifferences/solvers/fdm1dimsolver.cpp�20:#include <ql/math/interpolations/cubicinterpolation.hpp>
ql/methods/finitedifferences/solvers/fdm3dimsolver.cpp�21:#include <ql/math/interpolations/cubicinterpolation.hpp>
ql/methods/finitedifferences/utilities/localvolrndcalculator.cpp�28:#include <ql/math/interpolations/cubicinterpolation.hpp>
ql/methods/finitedifferences/stepconditions/fdmarithmeticaveragecondition.cpp�24:#include <ql/math/interpolations/cubicinterpolation.hpp>
ql/pricingengines/swaption/gaussian1dfloatfloatswaptionengine.cpp�22:#include <ql/math/interpolations/cubicinterpolation.hpp>
ql/pricingengines/swaption/gaussian1dnonstandardswaptionengine.cpp�24:#include <ql/math/interpolations/cubicinterpolation.hpp>
ql/pricingengines/swaption/gaussian1dswaptionengine.cpp�21:#include <ql/math/interpolations/cubicinterpolation.hpp>
ql/pricingengines/capfloor/gaussian1dcapfloorengine.cpp�21:#include <ql/math/interpolations/cubicinterpolation.hpp>
ql/models/shortrate/onefactormodels/markovfunctional.cpp�21:#include <ql/math/interpolations/cubicinterpolation.hpp>
ql/models/shortrate/onefactormodels/gaussian1dmodel.cpp�21:#include <ql/math/interpolations/cubicinterpolation.hpp>
ql/math/interpolations/cubicinterpolation.hpp�25:/*! \file cubicinterpolation.hpp
ql/math/interpolations/all.hpp�11:#include <ql/math/interpolations/cubicinterpolation.hpp>
ql/math/interpolations/loginterpolation.hpp�29:#include <ql/math/interpolations/cubicinterpolation.hpp>
ql/math/interpolations/mixedinterpolation.hpp�28:#include <ql/math/interpolations/cubicinterpolation.hpp>
ql/math/interpolations/bicubicsplineinterpolation.hpp�29:#include <ql/math/interpolations/cubicinterpolation.hpp>
ql/math/sampledcurve.hpp�29:#include <ql/math/interpolations/cubicinterpolation.hpp>
ql/termstructures/volatility/equityfx/andreasenhugevolatilityinterpl.cpp�25:#include <ql/math/interpolations/cubicinterpolation.hpp>
ql/termstructures/volatility/capfloor/capfloortermvolcurve.cpp�24:#include <ql/math/interpolations/cubicinterpolation.hpp>
ql/termstructures/volatility/optionlet/strippedoptionletadapter.cpp�28:#include <ql/math/interpolations/cubicinterpolation.hpp>
test-suite/piecewiseyieldcurve.cpp�32:#include <ql/math/interpolations/cubicinterpolation.hpp>
test-suite/interpolations.cpp�34:#include <ql/math/interpolations/cubicinterpolation.hpp>
test-suite/basismodels.cpp�34:#include <ql/math/interpolations/cubicinterpolation.hpp>
test-suite/fdmlinearop.cpp�34:#include <ql/math/interpolations/cubicinterpolation.hpp>
test-suite/piecewisezerospreadedtermstructure.cpp�30:#include <ql/math/interpolations/cubicinterpolation.hpp>
test-suite/inflationvolatility.cpp�23:#include <ql/math/interpolations/cubicinterpolation.hpp>

@pcaspers
Copy link
Contributor Author

Both are listed as expensive headers / template instantiations by ClangBuildAnalyzer.

I think we should remove incrementalstatistics.hpp from the list because of boostorg/accumulators#20. And if we decide to move the template instantiation from hpp to cpp the header will be much cheaper to parse / compile and not a candidate for the list anyhow I suppose.

It's probably similar for cubicinterpolation.hpp. If we move parts of the function implementation to cpp (as I did in #1787) it won't be necessary to include this header either.

@pcaspers
Copy link
Contributor Author

While testing this I notice that when using clang an additional flag -Xclang -fno-pch-timestamp is required. This is documented here: https://ccache.dev/manual/4.8.3.html#_precompiled_headers

@eltoder
Copy link
Contributor

eltoder commented Nov 21, 2023

@pcaspers Are you looking at full or incremental builds? If it's for the former, maybe it can be an option like unity builds. Maybe even combined with it.

@pcaspers
Copy link
Contributor Author

pcaspers commented Dec 7, 2023

I have added a cmake flag QL_USE_PCH in ORE so that we can make use of precompiled headers when building QuantLib and our extension libraries. Given that it does not universally work I have defaulted it of OFF for the time being. I don't think we need this change in QuantLib itself (unless @lballabio you want it?) since it's only adding a couple of lines to the cmake lists.

@pcaspers pcaspers closed this Dec 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants