Skip to content

Inline BH - #362

Merged
krlmlr merged 13 commits into
mainfrom
f-bh
Apr 11, 2021
Merged

Inline BH#362
krlmlr merged 13 commits into
mainfrom
f-bh

Conversation

@krlmlr

@krlmlr krlmlr commented Apr 5, 2021

Copy link
Copy Markdown
Member

Documentation follows. Key code chunks:

sudo apt install libboost1.71-tools-dev
# https://www.boost.org/doc/libs/1_75_0/tools/bcp/doc/html/index.html
mkdir -p src/vendor; bcp --boost=../../bh/inst/include --list-short src/*.cpp src/*.h
# Compiler flags: -Ivendor -MD, remove -MD later: https://stackoverflow.com/a/5835035/946850 (check out -H flag)
cd src; sed -r -i '/^[^ ]/ D; s/ \\$//g;s/(.) ([^\\])/\1\n \2/g' *.d
cat *.d | sort | uniq | grep -v '^ /' | xargs git add
# Keep unused files in separate branch, in case other compilers need it
ag -g gcc src/vendor
# Add files for other compilers
git checkout f-bh-unused -- $(ag -g gcc | xargs dirname | xargs dirname) src/vendor/boost/{predef,winapi,container_hash,integer/static_log2.hpp,detail/container_fwd.hpp,type_traits}

@hadley

hadley commented Apr 5, 2021

Copy link
Copy Markdown
Member

What are the other boost headers that we're using?

@krlmlr

krlmlr commented Apr 5, 2021

Copy link
Copy Markdown
Member Author
$ grep -h boost/ src/*.cpp src/*.h  | sort | uniq | xclip -se c
#include <boost/algorithm/string.hpp>
#include <boost/bind.hpp>
#include <boost/container/stable_vector.hpp>
#include <boost/cstdint.hpp>
#include <boost/date_time/gregorian/gregorian.hpp>
#include <boost/date_time/posix_time/posix_time.hpp>
#include <boost/limits.hpp>
#include <boost/noncopyable.hpp>
#include <boost/ptr_container/ptr_vector.hpp>
#include <boost/range/algorithm_ext/for_each.hpp>
#include <boost/scoped_ptr.hpp>
#include <boost/shared_ptr.hpp>

This is the minimal set to compile cleanly on Ubuntu. We need to add a few platform-specific headers to make it work on other platforms.

We can inline boost today and work on replacing the boost headers that are redundant with C++11 step by step.

@hadley

hadley commented Apr 5, 2021

Copy link
Copy Markdown
Member

Do we have an easy way to regenerate the minimal set of needed headers as we remove? If not, it might be easier to convert the low hanging fruit to C++11 before embedding boost?

@krlmlr

krlmlr commented Apr 5, 2021

Copy link
Copy Markdown
Member Author

We can easily recreate the minimal set.

I have compiled with -MD, this gives .d files that contain all headers used. Scriptlets are in the original post. We also need compiler-specific files, ag -g gcc | xargs dirname | unique sounds like a good approximation.

I'll document the process so that it's easy to apply everywhere.

@bdeitte

bdeitte commented Apr 7, 2021

Copy link
Copy Markdown

Hi @krlmlr, I ran this against RSPM binary build setup and nearly all of the 40 builds were successful. All CentOS 7 builds did fail though, with the errors below.

Against R 3.3, 3.4, 3.5 it fails with:

vendor/boost/type_traits/is_function.hpp:23:59: fatal error: boost/type_traits/detail/is_function_cxx_03.hpp: No such file or directory
#include <boost/type_traits/detail/is_function_cxx_03.hpp>

Against R 3.6, 4.0 it fails with:

vendor/boost/type_traits/detail/has_binary_operator.hpp:90:48: fatal error: boost/type_traits/is_fundamental.hpp: No such file or directory
#include <boost/type_traits/is_fundamental.hpp>

@krlmlr

krlmlr commented Apr 7, 2021

Copy link
Copy Markdown
Member Author

Thanks! I'll see if I can replicate with r-hub, and add the missing headers either way. I'll respond here.

@krlmlr

krlmlr commented Apr 8, 2021

Copy link
Copy Markdown
Member Author

@bdeitte: Thanks, I've added more headers, could you please recheck on CentOS 7? Unfortunately, rhub currently doesn't seem to support that platform.

@krlmlr

krlmlr commented Apr 8, 2021

Copy link
Copy Markdown
Member Author

I'm also wondering if it's worthwhile trying to determine the minimum set of necessary files. Currently this PR amounts to 18.5 MB of extra header files, if I include all relevant headers we'd add "only" 8.5 MB extra. It may be better to check which top-level headers actually bring in how many extra headers.

@bdeitte: If the current version doesn't build out of the box, could you please check the f-bh-full branch?

@krlmlr

krlmlr commented Apr 8, 2021

Copy link
Copy Markdown
Member Author

Turns out we don't save anything at all by eliminating the low-hanging fruit, we need to get rid of date_time first:

## # A tibble: 12 x 3
##    comb_id        size include                            
##      <int> <fs::bytes> <chr>                              
##  1      11     401.01K limits.hpp                         
##  2       2     401.27K noncopyable.hpp                    
##  3       8     419.07K cstdint.hpp                        
##  4       5     422.49K scoped_ptr.hpp                     
##  5       7     633.54K bind.hpp                           
##  6       1       1.25M shared_ptr.hpp                     
##  7       3       1.52M container/stable_vector.hpp        
##  8      12      11.81M range/algorithm_ext/for_each.hpp   
##  9       4      13.08M ptr_container/ptr_vector.hpp       
## 10       6      13.67M algorithm/string.hpp               
## 11       9      15.25M date_time/gregorian/gregorian.hpp  
## 12      10      15.72M date_time/posix_time/posix_time.hpp

Full analysis at https://rpubs.com/krlmlr/boost-sizes.

@bdeitte

bdeitte commented Apr 8, 2021

Copy link
Copy Markdown

I tried both of the variations that failed above, and the newer R version one is successful now. The older R version is failing with:

In file included from vendor/boost/bind.hpp:29:0,
                 from DbDataFrame.cpp:7:
vendor/boost/bind/bind.hpp:1428:40: fatal error: boost/bind/bind_template.hpp: No such file or directory
 #include <boost/bind/bind_template.hpp>

I then tried f-bh-full and that did work.

@bdeitte

bdeitte commented Apr 8, 2021

Copy link
Copy Markdown

Also just so it's clear here, the big issue that we hope to solve with inlining is not to reduce the size of all files included but the number of files. Of course, having everything smaller is nice too. 😄

@krlmlr

krlmlr commented Apr 8, 2021

Copy link
Copy Markdown
Member Author

Thanks. I see that without a full test suite for all relevant OSes it's not worthwhile minimizing the headers to be included.

I think if the binary is installed the files in src are not going to be installed. BH is different because it has its files in inst/include .

Right now Boost adds ~2300 files. In practice I think if users on slow file systems use binary packages they should be fine.

@hadley

hadley commented Apr 8, 2021

Copy link
Copy Markdown
Member

We could probably replace the date_time stuff either with clock, or by vendoring in the Date library that clock uses. I think that would also eliminate string.

@krlmlr
krlmlr merged commit fda5031 into main Apr 11, 2021
@krlmlr
krlmlr deleted the f-bh branch April 11, 2021 07:54
@krlmlr

krlmlr commented Apr 11, 2021

Copy link
Copy Markdown
Member Author

Thanks for your help!

@krlmlr krlmlr mentioned this pull request Aug 21, 2021
@NLDLEW1

NLDLEW1 commented Nov 10, 2021

Copy link
Copy Markdown

Hi, older versions of R (3.4.1, for instance) fail when compiling the current version due to a missing bind_template.hpp header. Branch f-bh-full compiles fine. Is there a specific reason this template file is not included in the main branch?

@krlmlr

krlmlr commented Nov 12, 2021

Copy link
Copy Markdown
Member Author

Thanks, can you please open a new issue? We check R 3.4.1 on CI/CD, what operating system are you using?

@krlmlr

krlmlr commented Nov 12, 2021

Copy link
Copy Markdown
Member Author

(Not R 3.4.1 but the latest R 3.4. This might make a difference indeed.)

@github-actions github-actions Bot locked as resolved and limited conversation to collaborators Nov 13, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants