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

Precompiled header with g++ #872

Closed
rok-cesnovar opened this issue May 11, 2020 · 5 comments · Fixed by #875
Closed

Precompiled header with g++ #872

rok-cesnovar opened this issue May 11, 2020 · 5 comments · Fixed by #875
Labels

Comments

@rok-cesnovar
Copy link
Member

rok-cesnovar commented May 11, 2020

Description:

We are currently using precompiled model headers (model_header.hpp.gch) only with clang. See lines 20-23 here: https://github.com/stan-dev/cmdstan/blob/develop/make/program#L20

We could do the same with g++ which would speedup g++ compilation of a model to match with clang - see #863 (comment).

I tried this out on Linux and Windows. Here is what I found out (I used the example bernoulli model):

  • Ubuntu g++ 7.2.0: compile time goes from 12.5s to 6s
  • Windows with Rtools 4.0 (g++ 8): compile time goes from 22s to 13s
  • Windows with Rtools 3.5 (g++ 4.9.3): does not work

The precompiled header with g++ does take up 500-700MB (the clang++ one takes 170MB on Linux). There is also a warning due to an Eigen issue that needs to be silenced with -Wno-ignored-attributes (see comment below).

Given the size of the precompiled header, the warnings that need to be turned off, the fact that it does not work on all of our supported systems and that we can't detect which compiler we use with makefiles, we have the following options:

  • A: use it by default on Linux and Mac (do people use g++ on a Mac at all?), and hide it behind a makefile flag (PRECOMPILED_HEADERS=true for example) so users on Windows with RTools 4.0 can use it.
  • B: hide it behind a makefile flag for g++ on all OSes
  • C: not bother with this, close issue

Current Version:

v2.23.0

@rok-cesnovar
Copy link
Member Author

I almost forgot, there is also a warning with g++:

In file included from stan/lib/stan_math/lib/eigen_3.3.7/Eigen/Core:434:0,
                 from stan/lib/stan_math/lib/eigen_3.3.7/Eigen/Dense:1,
                 from stan/lib/stan_math/stan/math/prim/fun/Eigen.hpp:22,
                 from stan/lib/stan_math/stan/math/rev.hpp:4,
                 from stan/lib/stan_math/stan/math.hpp:19,
                 from stan/src/stan/model/model_header.hpp:4:
stan/lib/stan_math/lib/eigen_3.3.7/Eigen/src/Core/DenseCoeffsBase.h: In instantiation of ‘class Eigen::DenseCoeffsBase<Eigen::CwiseUnaryOp<Eigen::internal::scalar_abs2_op<double>, const Eigen::Block<Eigen::Block<Eigen::Matrix<double, -1, -1>, -1, 1, true>, -1, 1, false> >, 0>’:
stan/lib/stan_math/lib/eigen_3.3.7/Eigen/src/Core/DenseBase.h:41:34:   required from ‘class Eigen::DenseBase<Eigen::CwiseUnaryOp<Eigen::internal::scalar_abs2_op<double>, const Eigen::Block<Eigen::Block<Eigen::Matrix<double, -1, -1>, -1, 1, true>, -1, 1, false> > >’
stan/lib/stan_math/lib/eigen_3.3.7/Eigen/src/Core/MatrixBase.h:48:34:   required from ‘class Eigen::MatrixBase<Eigen::CwiseUnaryOp<Eigen::internal::scalar_abs2_op<double>, const Eigen::Block<Eigen::Block<Eigen::Matrix<double, -1, -1>, -1, 1, true>, -1, 1, false> > >’
stan/lib/stan_math/lib/eigen_3.3.7/Eigen/src/Core/CwiseUnaryOp.h:94:7:   required from ‘class Eigen::CwiseUnaryOpImpl<Eigen::internal::scalar_abs2_op<double>, const Eigen::Block<Eigen::Block<Eigen::Matrix<double, -1, -1>, -1, 1, true>, -1, 1, false>, Eigen::Dense>’
stan/lib/stan_math/lib/eigen_3.3.7/Eigen/src/Core/CwiseUnaryOp.h:55:7:   required from ‘class Eigen::CwiseUnaryOp<Eigen::internal::scalar_abs2_op<double>, const Eigen::Block<Eigen::Block<Eigen::Matrix<double, -1, -1>, -1, 1, true>, -1, 1, false> >’
stan/lib/stan_math/lib/eigen_3.3.7/Eigen/src/Core/Dot.h:98:43:   required from ‘typename Eigen::NumTraits<typename Eigen::internal::traits<T>::Scalar>::Real Eigen::MatrixBase<Derived>::squaredNorm() const [with Derived = Eigen::Block<Eigen::Block<Eigen::Matrix<double, -1, -1>, -1, 1, true>, -1, 1, false>; typename Eigen::NumTraits<typename Eigen::internal::traits<T>::Scalar>::Real = double]’
stan/lib/stan_math/stan/math/prim/fun/multiply_lower_tri_self_transpose.hpp:34:47:   required from here
stan/lib/stan_math/lib/eigen_3.3.7/Eigen/src/Core/DenseCoeffsBase.h:55:30: warning: ignoring attributes on template argument ‘Eigen::internal::packet_traits<double>::type {aka __vector(2) double}’ [-Wignored-attributes]
                      >::type PacketReturnType;
                              ^~~~~~~~~~~~~~~~
stan/lib/stan_math/lib/eigen_3.3.7/Eigen/src/Core/DenseCoeffsBase.h: In instantiation of ‘class Eigen::DenseCoeffsBase<Eigen::Transpose<const Eigen::Transpose<const Eigen::Product<Eigen::Matrix<double, -1, -1>, Eigen::TriangularView<const Eigen::Transpose<const Eigen::Matrix<double, -1, -1> >, 2>, 0> > >, 0>’:
stan/lib/stan_math/lib/eigen_3.3.7/Eigen/src/Core/DenseBase.h:41:34:   required from ‘class Eigen::DenseBase<Eigen::Transpose<const Eigen::Transpose<const Eigen::Product<Eigen::Matrix<double, -1, -1>, Eigen::TriangularView<const Eigen::Transpose<const Eigen::Matrix<double, -1, -1> >, 2>, 0> > > >’
stan/lib/stan_math/lib/eigen_3.3.7/Eigen/src/Core/MatrixBase.h:48:34:   required from ‘class Eigen::MatrixBase<Eigen::Transpose<const Eigen::Transpose<const Eigen::Product<Eigen::Matrix<double, -1, -1>, Eigen::TriangularView<const Eigen::Transpose<const Eigen::Matrix<double, -1, -1> >, 2>, 0> > > >’
stan/lib/stan_math/lib/eigen_3.3.7/Eigen/src/Core/Transpose.h:115:37:   required from ‘class Eigen::TransposeImpl<const Eigen::Transpose<const Eigen::Product<Eigen::Matrix<double, -1, -1>, Eigen::TriangularView<const Eigen::Transpose<const Eigen::Matrix<double, -1, -1> >, 2>, 0> >, Eigen::Dense>’
stan/lib/stan_math/lib/eigen_3.3.7/Eigen/src/Core/Transpose.h:52:37:   required from ‘class Eigen::Transpose<const Eigen::Transpose<const Eigen::Product<Eigen::Matrix<double, -1, -1>, Eigen::TriangularView<const Eigen::Transpose<const Eigen::Matrix<double, -1, -1> >, 2>, 0> > >’
stan/lib/stan_math/stan/math/prim/fun/tcrossprod.hpp:27:14:   required from ‘Eigen::Matrix<typename stan::value_type<T>::type, T:: RowsAtCompileTime, T:: RowsAtCompileTime> stan::math::tcrossprod(const T&) [with T = Eigen::Transpose<const Eigen::Product<Eigen::Matrix<double, -1, -1>, Eigen::TriangularView<const Eigen::Transpose<const Eigen::Matrix<double, -1, -1> >, 2>, 0> >; stan::require_eigen_vt<std::is_arithmetic, T>* <anonymous> = 0; typename stan::value_type<T>::type = double]’
stan/lib/stan_math/stan/math/prim/fun/crossprod.hpp:21:20:   required from ‘auto stan::math::crossprod(const EigMat&) [with EigMat = Eigen::Product<Eigen::Matrix<double, -1, -1>, Eigen::TriangularView<const Eigen::Transpose<const Eigen::Matrix<double, -1, -1> >, 2>, 0>; stan::require_eigen_t<T>* <anonymous> = 0]’
stan/lib/stan_math/stan/math/prim/prob/wishart_rng.hpp:30:41:   required from here
stan/lib/stan_math/lib/eigen_3.3.7/Eigen/src/Core/DenseCoeffsBase.h:55:30: warning: ignoring attributes on template argument ‘Eigen::internal::packet_traits<double>::type {aka __vector(2) double}’ [-Wignored-attributes]
stan/lib/stan_math/lib/eigen_3.3.7/Eigen/src/Core/DenseCoeffsBase.h: In instantiation of ‘class Eigen::DenseCoeffsBase<Eigen::Product<Eigen::Transpose<const Eigen::Product<Eigen::Matrix<double, -1, -1>, Eigen::TriangularView<const Eigen::Transpose<const Eigen::Matrix<double, -1, -1> >, 2>, 0> >, Eigen::Transpose<const Eigen::Transpose<const Eigen::Product<Eigen::Matrix<double, -1, -1>, Eigen::TriangularView<const Eigen::Transpose<const Eigen::Matrix<double, -1, -1> >, 2>, 0> > >, 0>, 0>’:
stan/lib/stan_math/lib/eigen_3.3.7/Eigen/src/Core/DenseBase.h:41:34:   required from ‘class Eigen::DenseBase<Eigen::Product<Eigen::Transpose<const Eigen::Product<Eigen::Matrix<double, -1, -1>, Eigen::TriangularView<const Eigen::Transpose<const Eigen::Matrix<double, -1, -1> >, 2>, 0> >, Eigen::Transpose<const Eigen::Transpose<const Eigen::Product<Eigen::Matrix<double, -1, -1>, Eigen::TriangularView<const Eigen::Transpose<const Eigen::Matrix<double, -1, -1> >, 2>, 0> > >, 0> >’
stan/lib/stan_math/lib/eigen_3.3.7/Eigen/src/Core/MatrixBase.h:48:34:   required from ‘class Eigen::MatrixBase<Eigen::Product<Eigen::Transpose<const Eigen::Product<Eigen::Matrix<double, -1, -1>, Eigen::TriangularView<const Eigen::Transpose<const Eigen::Matrix<double, -1, -1> >, 2>, 0> >, Eigen::Transpose<const Eigen::Transpose<const Eigen::Product<Eigen::Matrix<double, -1, -1>, Eigen::TriangularView<const Eigen::Transpose<const Eigen::Matrix<double, -1, -1> >, 2>, 0> > >, 0> >’
stan/lib/stan_math/lib/eigen_3.3.7/Eigen/src/Core/Product.h:115:7:   required from ‘class Eigen::internal::dense_product_base<Eigen::Transpose<const Eigen::Product<Eigen::Matrix<double, -1, -1>, Eigen::TriangularView<const Eigen::Transpose<const Eigen::Matrix<double, -1, -1> >, 2>, 0> >, Eigen::Transpose<const Eigen::Transpose<const Eigen::Product<Eigen::Matrix<double, -1, -1>, Eigen::TriangularView<const Eigen::Transpose<const Eigen::Matrix<double, -1, -1> >, 2>, 0> > >, 0, 8>’
stan/lib/stan_math/lib/eigen_3.3.7/Eigen/src/Core/Product.h:147:7:   required from ‘class Eigen::ProductImpl<Eigen::Transpose<const Eigen::Product<Eigen::Matrix<double, -1, -1>, Eigen::TriangularView<const Eigen::Transpose<const Eigen::Matrix<double, -1, -1> >, 2>, 0> >, Eigen::Transpose<const Eigen::Transpose<const Eigen::Product<Eigen::Matrix<double, -1, -1>, Eigen::TriangularView<const Eigen::Transpose<const Eigen::Matrix<double, -1, -1> >, 2>, 0> > >, 0, Eigen::Dense>’
stan/lib/stan_math/lib/eigen_3.3.7/Eigen/src/Core/Product.h:71:7:   required from ‘class Eigen::Product<Eigen::Transpose<const Eigen::Product<Eigen::Matrix<double, -1, -1>, Eigen::TriangularView<const Eigen::Transpose<const Eigen::Matrix<double, -1, -1> >, 2>, 0> >, Eigen::Transpose<const Eigen::Transpose<const Eigen::Product<Eigen::Matrix<double, -1, -1>, Eigen::TriangularView<const Eigen::Transpose<const Eigen::Matrix<double, -1, -1> >, 2>, 0> > >, 0>’
stan/lib/stan_math/stan/math/prim/fun/tcrossprod.hpp:27:14:   required from ‘Eigen::Matrix<typename stan::value_type<T>::type, T:: RowsAtCompileTime, T:: RowsAtCompileTime> stan::math::tcrossprod(const T&) [with T = Eigen::Transpose<const Eigen::Product<Eigen::Matrix<double, -1, -1>, Eigen::TriangularView<const Eigen::Transpose<const Eigen::Matrix<double, -1, -1> >, 2>, 0> >; stan::require_eigen_vt<std::is_arithmetic, T>* <anonymous> = 0; typename stan::value_type<T>::type = double]’
stan/lib/stan_math/stan/math/prim/fun/crossprod.hpp:21:20:   required from ‘auto stan::math::crossprod(const EigMat&) [with EigMat = Eigen::Product<Eigen::Matrix<double, -1, -1>, Eigen::TriangularView<const Eigen::Transpose<const Eigen::Matrix<double, -1, -1> >, 2>, 0>; stan::require_eigen_t<T>* <anonymous> = 0]’
stan/lib/stan_math/stan/math/prim/prob/wishart_rng.hpp:30:41:   required from here
stan/lib/stan_math/lib/eigen_3.3.7/Eigen/src/Core/DenseCoeffsBase.h:55:30: warning: ignoring attributes on template argument ‘Eigen::internal::packet_traits<double>::type {aka __vector(2) double}’ [-Wignored-attributes]

Its an Eigen issue: https://gitlab.com/libeigen/eigen/-/issues/1221
They propose setting -Wno-ignored-attributes to compile flags. Then it goes away.
It has no effect on performance.

@wds15
Copy link
Contributor

wds15 commented May 11, 2020

option A sounds great.

I would say that on Mac everyone uses clang is a fair statement.

@mitzimorris
Copy link
Member

mitzimorris commented May 11, 2020 via email

@rok-cesnovar
Copy link
Member Author

On mac/linux with clang: no change, precompiled headers are already used
On mac/linux with g++: precompiled headers would now be built and used

On Windows without the flag: no change
Windows with flag: precompiled headers would be built and used (works with RTools 4 only)

CmdstanR/Py could help windows users by setting it automatically based on detected toolchain.

@bob-carpenter
Copy link
Contributor

I like solution (A) and think it's well worth doing. It'll also have the nice side effect of encouraging Windows users to upgrade RTools.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants