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

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

Closed
jeroen opened this issue Aug 26, 2023 · 10 comments
Closed

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

jeroen opened this issue Aug 26, 2023 · 10 comments

Comments

@jeroen
Copy link
Member

jeroen commented Aug 26, 2023

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
Copy link
Member

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
Copy link
Member

krlmlr commented Sep 3, 2023

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
Copy link

Enchufa2 commented Sep 9, 2023

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
Copy link

Enchufa2 commented Sep 9, 2023

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
Copy link

Enchufa2 commented Sep 9, 2023

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- added a commit that referenced this issue Sep 10, 2023
@lionel-
Copy link
Member

lionel- commented Sep 10, 2023

@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-
Copy link
Member

lionel- commented Sep 10, 2023

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

langmm added a commit to cropsinsilico/yggdrasil that referenced this issue Sep 12, 2023
moralapablo pushed a commit to IBiDat/nn2poly that referenced this issue Sep 18, 2023
* This commit aims to solve issue 41.

Now the default value for `q_taylor_vector` is 8 for non linear layers and 1 for the linear ones.
Also, check weight constraints has been removed from exports and a new internal function called `check_weight_dimensions` has been created.

Additional tests have also been added.

* pin to vdiffr@1.0.5 (see r-lib/vdiffr/issues/137)

---------

Co-authored-by: Iñaki Úcar <iucar@fedoraproject.org>
@trevorld
Copy link

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-
Copy link
Member

lionel- commented Sep 22, 2023

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

@lionel-
Copy link
Member

lionel- commented Sep 22, 2023

On CRAN!

@lionel- lionel- closed this as completed Sep 22, 2023
vandenman added a commit to jasp-stats/jasp-actions that referenced this issue Oct 13, 2023
vdiffr 1.0.7 is out and r-lib/vdiffr#137 is closed so we no longer need this.
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

No branches or pull requests

6 participants