r-lib / vdiffr

Visual regression testing and graphical diffing with testthat
https://vdiffr.r-lib.org
Other
180 stars 31 forks source link

OOM compiling devSVG.cpp in C++17 #137

Closed jeroen closed 9 months ago

jeroen commented 10 months ago

Since the switch to c++17 your CI hangs trying to compile devSVG.cpp for r-release and r-devel and also pkgdown. The latest versions of R now default to C++17, which causes a massive memory usage, at least on Ubuntu 22.04.

Tested locally that the compilation of devSVG.cpp requires about 12GB mem and 10 minutes on ubuntu-22.04 in x64. I am not sure if this is a bug in GCC or expected. I am seeing the same problem on r-universe:

* installing to library ‘/usr/local/lib/R/site-library’
  * installing *source* package ‘vdiffr’ ...
  ** using staged installation
  ** libs
  using C++ compiler: ‘g++ (Ubuntu 11.4.0-1ubuntu1~22.04) 11.4.0’
  g++ -std=gnu++17 -I"/usr/share/R/include" -DNDEBUG  -I'/usr/local/lib/R/site-library/cpp11/include'     -fpic  -O2 -ffile-prefix-map=/build/r-base-MHXHhT/r-base-4.3.1=. -fstack-protector-strong -Wformat -Werror=format-security -Wdate-time -D_FORTIFY_SOURCE=2  -c compare.cpp -o compare.o
  g++ -std=gnu++17 -I"/usr/share/R/include" -DNDEBUG  -I'/usr/local/lib/R/site-library/cpp11/include'     -fpic  -O2 -ffile-prefix-map=/build/r-base-MHXHhT/r-base-4.3.1=. -fstack-protector-strong -Wformat -Werror=format-security -Wdate-time -D_FORTIFY_SOURCE=2  -c cpp11.cpp -o cpp11.o
  g++ -std=gnu++17 -I"/usr/share/R/include" -DNDEBUG  -I'/usr/local/lib/R/site-library/cpp11/include'     -fpic  -O2 -ffile-prefix-map=/build/r-base-MHXHhT/r-base-4.3.1=. -fstack-protector-strong -Wformat -Werror=format-security -Wdate-time -D_FORTIFY_SOURCE=2  -c devSVG.cpp -o devSVG.o
  g++: fatal error: Killed signal terminated program cc1plus
  compilation terminated.
  make: *** [/usr/lib/R/etc/Makeconf:200: devSVG.o] Error 1
  ERROR: compilation failed for package ‘vdiffr’
  * removing ‘/usr/local/lib/R/site-library/vdiffr’
thomasp85 commented 10 months ago

I must admit this is above my head but the easy fix seems to turn C++11 back on and inform CRAN of the reasons?

krlmlr commented 10 months ago

~IIUC, you don't need to communicate C++11 in DESCRIPTION, perhaps setting CXXSTD in Makevars is sufficient?~

CRAN does not want this, and removing this is exactly what caused the problem

Enchufa2 commented 10 months ago

What's the gcc version there? I tested this on Fedora with gcc 13.2.1 and, while the compilation takes a lot more time with C++17 compared to e.g. C++14, it takes only a minimal amount of memory.

Enchufa2 commented 10 months ago

Just checked on a rocker/r-ubuntu:jammy container with gcc 11.3.0, and I can confirm that the memory usage goes nuts after a few seconds.

Enchufa2 commented 10 months ago

I think the most reasonable fix is to add CXX_STD = CXX14 to the Makevars files. It was necessary to remove C++11 from SystemRequirements, but CRAN allows to set C++14 in the Makevars AFAIK.

lionel- commented 10 months ago

@Enchufa2 Good idea but on old R versions on Windows I'm getting:

*** arch - i386
Error in .shlib_internal(args) : 
  C++14 standard requested but CXX14 is not defined

I'll probably go back to C++11.

lionel- commented 10 months ago

Thank you for helping pinpoint the problematic GCC versions by the way. Good to know it seems fixed with more recent GCCs.

trevorld commented 9 months ago

FYI: "switching" the gcc used by Github Actions from the default 11.4.0 to 12.3.0 seems to work for me:

      - name: Upgrade gcc to compile vdiffr without error
        if: runner.os == 'Linux'
        run: |
            sudo apt-get install -y g++-12 gcc-12 # probably already installed
            sudo update-alternatives --install /usr/bin/g++ g++ /usr/bin/g++-12 60
            sudo update-alternatives --install /usr/bin/gcc gcc /usr/bin/gcc-12 60
lionel- commented 9 months ago

I'm trying to get a vdiffr update on CRAN, and this time it looks like it will go through :-)

lionel- commented 9 months ago

On CRAN!