Future rstan is broken with clang++

Developers
61

The last two commits from me to stan-math StanHeaders_2.21 branch do this:

  • The TBB is not used at all in cases whenever STAN_THREADS is not defined. The TBB is used if STAN_THREADS is defined.
  • The stan::math::lgamma now always uses the boost implementation as I was not able to get things to work otherwise. It appears as if _REENTRANT gets somewhere define after including cmath. If that happens, then we have a catastrophe which causes cmath to not export lgamma_r, but stan-math will think it is defined. This means a ~5% performance it or so for Linux&macOS for models using this function heavily.

With the above changes I can have StanHeaders 2.21.0 and rstan 2.19.2 installed and successfully compile & load RBesT.

For the lgamma stuff I suggest that we take the small performance hit for now and tell people to use the new _REENTRANT flag when they build their package (so put it in src/Makevars). As alternative we may introduce a STAN_HAS_LGAMMA_R macro which people can set if they are sure that lgamma_r is available.

Before I forget: Have you backported the boost::math::lgamma fixes which avoid overflow for large arguments of lgamma? That would be needed to get finite results from that function for arguments beyond 10^{25}.

Thanks for adding me to the author list.

62

OK. I am getting the same results now. Which boost::math::lgamma commit(s) do I need to backport to rstan?

63

It’s this commit:

You need to be able to run the stan-math tests for lgamma. These do call lgamma from boost directly and they only work if you have a patched boost in use.

The boost files involved are

  • lib/boost_1.69.0/boost/math/special_functions/detail/lanczos_sse2.hpp
  • lib/boost_1.69.0/boost/math/special_functions/gamma.hpp

BTW… we will not get the full benefits of faster compilation as we do not distribute the pre-compiled headers (for good reason as these blow up the binary by ~100MB). However, what would you say about the following approach: The standard is to not use the pre-compiled headers, but whenever people build the binary rstan package on their systems and define NOT_CRAN=true then we build during the compilation phase the pre-compiled headers which we use then during building the models. This way only people who build rstan from source in a specific will get this feature. This would be a quick and dirty way to include this functionality in a semi-official way and we can improve further later on. Makes sense?

64

OK, I can supercede those Boost files.

I do think having an option for precompiled headers would be good, but we may need to make a meta-header in rstan to facilitate that.

65

Ok… I will have a look, but that’s optional for releasing the first 21 I suppose.

Let’s see what Mac r experts say :

https://stat.ethz.ch/pipermail/r-sig-mac/2019-November/013116.html

1 Like
66

I have yet another fix for the Eigen plugin problem

that uses the zzz.R file to set PKG_CPPFLAGS. In principle, we could also put -D_REENTRANT there, and it would be set for everything except OpenMx and nlmixr (which do not use rstan and therefore do not load StanHeaders) but I have separate PRs for those two packages.

67

Hm… I tried the pre-compiled headers thing for rstan. So I did generate a model_header.hpp.gch which I did feed into clang accordingly. For the mvn_orange example I get these compilation times:

  • cmdstan 2.21: 17.5s
  • rstan 2.19: 40s
  • rstan 2.21: 27.2s
  • rstan 2.21 with pre-compiled: 23s

So cmdstan is clearly best and rstan 2.21 is a huge win over 2.19, but adding pre-compiled headers to rstan is not that much of a gain right now. We probably need to review in detail to see where the difference is coming from, but I would say we just leave it as is for rstan for the moment.