Split pybind11 into headers and cpp files to speedup compilation

[cirosantilli2]

Edit: I updated it to all files. But some time passed, so a massive rebase will be needed, which I'll do when I get the greenlight from contribs that the unrebased patch is the approach that we want.

TODO Fo now, only pybind11.h and options.h have been split. If maintainers
feel that this is going in the right direction, I will update this commit
to split all files.

As discussed at #2322, the split
is opt-in, and has been observed to speed up the compilation of the gem5
project by around 40% from 25 minutes to 15 minutes.

If the PYBIND11_DECLARATIONS_ONLY is not defined, then the .cpp files are
included in the .h files, and everything works header-only as was the case
before this commit.

If PYBIND11_DECLARATIONS_ONLY=1, then only declarations are made visible
from, the header files, and the user must the user must compile the
pybind11 .cpp files, also using PYBIND11_DECLARATIONS_ONLY=1, into objects
and add link those into the final link. This commit also updates CMakeLists
to automate building those object files.

This commit has been tested as follows.

First, the original build and pytest (without PYBIND11_DECLARATIONS_ONLY)
work as before:

mkdir build
cmake ..
make -j `nproc`
make pytest

TODO: if this commit gets traction, I will try to add a test for the
PYBIND11_DECLARATIONS_ONLY version too, otherwise it will likely break.
I'd just re-run the entire pybind with PYBIND11_DECLARATIONS_ONLY as well
every time.

The commit has also been tested with the following minimal manual example:

class_test.cpp

struct ClassTest {
    ClassTest(const std::string &name) : name(name) {}
    void setName(const std::string &name_) { name = name_; }
    const std::string &getName() const { return name; }
    std::string name;
};

struct ClassTestDerived : ClassTest {
    ClassTestDerived(const std::string &name, const std::string &name2) :
        ClassTest(name), name2(name2) {}
    std::string getName2() { return name + name2 + "2"; }
    std::string name2;
};

namespace py = pybind11;

PYBIND11_MODULE(class_test, m) {
    m.doc() = "pybind11 example plugin";
    py::class_<ClassTest>(m, "ClassTest")
        .def(py::init<const std::string &>())
        .def("setName", &ClassTest::setName)
        .def("getName", &ClassTest::getName)
        .def_readwrite("name", &ClassTest::name);
    py::class_<ClassTestDerived, ClassTest>(m, "ClassTestDerived")
        .def(py::init<const std::string &, const std::string &>())
        .def("getName2", &ClassTestDerived::getName2)
        .def_readwrite("name", &ClassTestDerived::name);
}

class_test_main.py

import class_test

my_class_test = class_test.ClassTest('abc');
assert(my_class_test.getName() == 'abc')
my_class_test.setName('012')
assert(my_class_test.name == '012')

my_class_test_derived = class_test.ClassTestDerived('abc', 'def');
assert(my_class_test_derived.getName2() == 'abcdef2')

without PYBIND11_DECLARATIONS_ONLY (python3-config --cflags) is opened up
and hacked to point to the custom pybind11 source:

g++ \
  -save-temps \
  -I/usr/include/python3.8 \
  -I/home/ciro/git/pybind11/include \
  -Wno-unused-result \
  -Wsign-compare \
  -g \
  -fdebug-prefix-map=/build/python3.8-fKk4GY/python3.8-3.8.2=. \
  -specs=/usr/share/dpkg/no-pie-compile.specs \
  -fstack-protector \
  -Wformat \
  -Werror=format-security  \
  -DNDEBUG \
  -g \
  -fwrapv \
  -O3 \
  -Wall \
  -shared \
  -std=c++11 \
  -fPIC class_test.cpp \
  -o class_test`python3-config --extension-suffix` \
  `python3-config --libs` \
;
./class_test_main.py

with PYBIND11_DECLARATIONS_ONLY:

g++ \
  -DPYBIND11_DECLARATIONS_ONLY=1 \
  -save-temps \
  -I/usr/include/python3.8 \
  -I/home/ciro/git/pybind11/include \
  -Wno-unused-result \
  -Wsign-compare \
  -g \
  -fdebug-prefix-map=/build/python3.8-fKk4GY/python3.8-3.8.2=. \
  -specs=/usr/share/dpkg/no-pie-compile.specs \
  -fstack-protector \
  -Wformat \
  -Werror=format-security  \
  -DNDEBUG \
  -g \
  -fwrapv \
  -O3 \
  -Wall \
  -shared \
  -std=c++11 \
  -fPIC class_test.cpp \
  /home/ciro/git/pybind11/build/libpybind11.a \
  -o class_test`python3-config --extension-suffix` \
  `python3-config --libs` \
;
./class_test_main.py

[henryiii]

I'm happy to help with the CMake setup if this continues. :)

[YannickJadoul]

Still an interesting discussion to be had: where do we draw the line. I.e., even in "normal" C++ code, split up as usual over headers and cpp-files, this constructor might have been declared here.

I guess this won't be the part adding compilation overhead?

[YannickJadoul]

Same with the other functions, actually. This might be a part that's good to keep as is, and to have inline even in a build with source files.

[cirosantilli2]

In this particular case, it is not immediately obvious if the function is trivial or not, because global_state() could in theory expand to a huge inlined function (although this is unlikely since large functions are generally not inlined). A better rule might be to keep in headers only constructor that perform only trivial assignments.

[YannickJadoul]

Leaving aside the organization of the files (cpp files shouldn't be in include, I expect? I don't know how other projects do this), the build system (luckily we have @henryiii for this, willing to help out, so I'm not worried about this :-) ), and some bikeshedding on names (PYBIND11_DECLARATIONS_ONLY is better, but still not a fan; if I think of something better, I'll shout, but this is not urgent in the current context), I think this all looks pretty good :-)

The more I'm making PRs and compiling (and running) pybind11's test suite, the more I think I really want this PR to go in ;-)

The one thing we should still discuss is what inline declarations are still fine to keep in the headers. I'm thinking of empty constructors (maybe they should become = default; actually), or things as simple as setValue(int value) { m_value = value; }. Also in non-header-only projects, these are often left in the definition of a class in the header, no? So it would be worth testing out how much they influence the time it takes to compile; I'm hoping it'd be negligible.

[YannickJadoul]

Same here. This might just stay as it is. The real gain comes from things like initialize_generic, right?

[YannickJadoul]

Btw, compilation is failing because options.cpp includes options.h includes options.cpp (and the cpp file obviously doesn't have an include guard, because it's a cpp file).

[henryiii]

I think .inc is used sometimes, but may not get nice highlighting in some editors. fmtlib uses <name>-inl.h, which I think is my favorite. /src then has cpp files that mostly just include the *-inl.h files.

[YannickJadoul]

Sounds good to me! That should solve the circular dependency problem and the src vs. include directory thing.

[cirosantilli2]

I think .inc is used sometimes, but may not get nice highlighting in some editors. fmtlib uses -inl.h, which I think is my favorite. /src then has cpp files that mostly just include the *-inl.h files.

Ah, I found the problem, I had renamed HEADERS_ONLY to DECLARATIONS_ONLY in the last moment before pushing but forgot to do it in the CMakeLists.txt.

With the correct name -DPYBIND11_DECLARATIONS_ONLY=1 is passed in .cpp compilation, which breaks circularity due to the guards in the header:

#if !PYBIND11_DECLARATIONS_ONLY
#include "options.cpp"
#endif

Passing -DPYBIND11_DECLARATIONS_ONLY=1 when compiling the .cpp files is necessary not only because of this, but also because we need to disable inline, otherwise no definitions are made visible on the object files.

I would not recommend splitting yet another file to reduce maintenance overhead.

[cirosantilli2]

I'm happy to help with the CMake setup if this continues. :)

Thanks, Henry, I might need it later on!

Does the offer extends to quantum field theory help as well? Just kidding! 😆 I'm now at the point of trying to understand why can't we just use partial differential equations like Dirac's equation as done in other proper branches of physics for the model/whatever it is that you use instead 😆 One day, one day.

The one thing we should still discuss is what inline declarations are still fine to keep in the headers. I'm thinking of empty constructors (maybe they should become = default; actually), or things as simple as setValue(int value) { m_value = value; }. Also in non-header-only projects, these are often left in the definition of a class in the header, no? So it would be worth testing out how much they influence the time it takes to compile; I'm hoping it'd be negligible.

OK, I'll try to leave all empty contructors/trivial getters/setters in and check if the compilation time problem is solved (I expect like you that it will).

[cirosantilli2]

OK, the cmake help might be needed now XD The new failures are for make test_cmake_build which appears to test other projects using pybind11 via cmake. I tried to add +add_library(pybind11 EXCLUDE_FROM_ALL as a workaround but still fails. The .cpp files are however present in mock_install.

[henryiii]

I would not recommend splitting yet another file to reduce maintenance overhead.

I don't like the idea of having an #include <*.cpp> file, or *.cpp files being distributed as headers. Personally I find the fmtlib solution with simple *.cpp files that include the -inl.h files attractive. And you don't have to build inside the header directory (which is weird), so you can then put the CMake files in the src directory along with the .cpp files. You also get to fix any issues with pre-decleration, since the .cpp files can include what they need. Also, if you have something that is .cpp only, you can put it in the cpp instead of having more #ifs everywhere.

Better separation reduces maintenance overhead, too.

[YannickJadoul]

I do think I agree with @henryiii, here. Also, fmtlib instantiates some templates in the cpp file, I saw, so this structure gives us a bit of extra space to play around there.

But we can do this step by step and try this out only later, afaic, if you prefer first continuing as you started.

[cirosantilli2]

OK, I'm going to do another round of coding now :-)

I don't like the idea of having an #include <*.cpp> file, or *.cpp files being distributed as headers. Personally I find the fmtlib solution with simple *.cpp files that include the -inl.h files attractive. And you don't have to build inside the header directory (which is weird), so you can then put the CMake files in the src directory along with the .cpp files. You also get to fix any issues with pre-decleration, since the .cpp files can include what they need. Also, if you have something that is .cpp only, you can put it in the cpp instead of having more #ifs everywhere.

About .cpp in include, yes, splitting a file is the only solution, so if you guys think it's worth if for that (I don't, but bikeshedding), I'll do it, no problem.

About saving ifdefs however, I don't think any of the current boilerplate would be reduced by that change, but rather slightly increased.

The current boilerplate is:

include/*.h:

#if !PYBIND11_DECLARATIONS_ONLY
#include "options.cpp"
#endif

include/*.cpp

#include "options.h"

detail/common.h

#if PYBIND11_DECLARATIONS_ONLY
#  define PYBIND11_INLINE
#else
#  define PYBIND11_INLINE inline
#endif

and you must compile with -DPYBIND11_DECLARATIONS_ONLY=1.

What I understand, is that if I created a src/ directory split, things would look like:

src/options.cpp: a new file with two new lines added:

#include "options.h"
#include "options-inl.h"

.h: we still need to conditionally include the implementation on that header as before (now would be #include "options-inl.h" instead of a .cpp), otherwise it would not be usable as a header-only library anymore, so no net change

.inl.h: same as the old .cpp, but we can remove the #include "options.h", so one line gained.

detail/common.h: we still need PYBIND11_INLINE to be conditional as before for the same reasons (since the .h has to include the .inl.h, it does not matter where the inlines are placed, as the inline will would be seen in either case. I just keep them on .cpp because there an easy rule: every function has to have it there).

And you must still compile with -DPYBIND11_DECLARATIONS_ONLY=1. to turn off inline. Alternatively we could also instead define that on every .cpp file:

#define PYBIND11_DECLARATIONS_ONLY 1
#include "options.h"
#include "options-inl.h"

but that further increases the boilerplate on several files.

Let me know if I got anything above wrong/if there's a better way.

[cirosantilli2]

OK, I've converted all files to .cpp now, and previous testing state is unchanged.

If after my last comment you still feel it is better to split in -inl.h + .cpp, I'll do it on the next push, which will be needed because every new master commit is a merge conflict, yay!

As before cmake --build build --target test_cmake_build is failing the CI, it appears to test using this project from CMake. If anyone has any tips there, that would be amazing, as this is the CMake-specific part which I'm clueless about.

About keeping small functions on the header, I've been very strict and kept only:

• simple direct get/setters
• constructors that only directly set variables

and not for other things that might be considered trivial, like return a == b. If you have a different guideline, let me know. The current one has the advantage of being easy to follow without thinking :-)

[henryiii]

I'm still in favor of -inl.h because: a) having a .cpp file in /include and shipped with your library in packaging is odd, b) compilers sometimes complain if you put an include guard on a .cpp file (which is weird), c) you have the option of adding compile-only logic if needed, without extra includes, and d) at least one other library uses this model (fmtlib). But please only consider my opinion one vote, not a requirement.

because every new master commit is a merge conflict, yay!

This PR probably should focus on the procedure, structure, and extra parts (like CMake), as we may need to pause all PRs for a time while this actually finalizes and goes in. Might be a good time to close all open PRs since they will all need serious rebasing, since all code has moved (basically). I'm assuming this would be a 3.0 level feature?

[cirosantilli2]

I'm still in favor of -inl.h because: a) having a .cpp file in /include and shipped with your library in packaging is odd, b) compilers sometimes complain if you put an include guard on a .cpp file (which is weird), c) you have the option of adding compile-only logic if needed, without extra includes, and d) at least one other library uses this model (fmtlib). But please only consider my opinion one vote, not a requirement.

OK, I'll split like that in the next one unless other people argue otherwise 👍

b) by compiler guards do you mean #pragma once? On the original development, I had used them and noticed that it does gives warnings on .cpp. But then I noticed that they are not needed on the .cpp because we are forced to pass -DPYBIND11_DECLARATIONS_ONLY=1 while building .cpp to disable inline through PYBIND11_INLINE, and that breaks the circular dependency. So the current version does not have them, they are only used on .h as before

c) do you have any example in mind? Notably, what could we add to the .cpp file that would not break the headerness-only of the library? As things are looking now (this changed a bit from my previous comments after some thought), in the inl.h, the .cpp file will contain exactly one line: #include "options-inl.h" etc.

because every new master commit is a merge conflict, yay!

This PR probably should focus on the procedure, structure, and extra parts (like CMake), as we may need to pause all PRs for a time while this actually finalizes and goes in. Might be a good time to close all open PRs since they will all need serious rebasing, since all code has moved (basically). I'm assuming this would be a 3.0 level feature?

Just to clarify, I was a bit anxious to do the full conversion to ensure that it was still going to work with gem5 and have the speedup, but I checked that now and it worked :-)

Like you said, we should do the review here, and then I'll do a rebase after. But if other things can be paused, I wont complain 😁

One problem is that the automated tests don't seem to run if there are conflicts? But like I said, the next hopefully last "test blocker" is the cmake --build build --target test_cmake_build.

[henryiii]

The automated tests build on the PR merged with the branch it's targeting. So if there are conflicts, it can't run. You can manually run form your fork, though, just click "actions" in your fork, then the workflow to run (CI), and there will be a manual trigger dropdown there.

[henryiii]

b) I think, especially as we simplify this (I think pybind11 will actually have a few nice simplifications and dropped predeclarations when this is in place), that not having include guards on files you are including is likely dangerous.
c) Not really, I just noticed fmtlib takes advantage of this.

If we do the inl.h route, it's also easy to fuze the compile if there are files that should compile together. There's a significant memory/time overhead in setting up a compile unit, so being able to list multiple inl.h's in one file might be a useful method for optimizing the build.

[henryiii]

Also, CMake assume .cpp are built, so you'll have to loop over all the included files and force them to be considered headers to get it to work properly in header-only mode if you put .cpp's that are really optionally compiled headers.

[henryiii]

Added basic CMake support. You'll need to fix ../include/pybind11/cast.h:1458:15: warning: inline function 'pybind11::literals::operator""_a' is not defined [-Wundefined-inline] I believe, though.

[cirosantilli2]

Thanks so much for the cmake update @henryiii !!!

I squashed with my commit, and I also had to add a new commit to make things work with gem5 with missing #if, but I'll split that to a separate PR soon.

But when cmake --build build --target test_cmake_build I tried it failed with:

Built target test_build_installed_target
Scanning dependencies of target test_build_subdirectory_target
Internal cmake changing into directory: /home/ciro/git/pybind11/build/tests/test_cmake_build/subdirectory_target
Error: cmake execution failed
The CXX compiler identification is GNU 9.3.0
Check for working CXX compiler: /usr/bin/c++
Check for working CXX compiler: /usr/bin/c++ -- works
Detecting CXX compiler ABI info
Detecting CXX compiler ABI info - done
Detecting CXX compile features
Detecting CXX compile features - done
pybind11 v2.6.0 dev
Found PythonInterp: /usr/bin/python3.8 (found version "3.8.2") 
Found PythonLibs: /usr/lib/x86_64-linux-gnu/libpython3.8.so
Performing Test HAS_FLTO
Performing Test HAS_FLTO - Success
Using pybind11: (version "2.6.0" dev)
Configuring done
CMake Error: Cannot determine link language for target "pybind11_lib".
CMake Error: CMake can not determine linker language for target: pybind11_lib
Generating done
CMake Generate step failed.  Build files cannot be regenerated correctly.

Maybe I messed something up, but the cmake changes appear to be like you made them. Can you double check?

[cirosantilli2]

OK, I split the second unrelated commit to #2476 now.

[cirosantilli2]

OK, after hitting my head a bit more and trying a bunch of stuff, I managed to get Doxygen and therefore all tests passing: https://github.com/cirosantilli2/pybind11/actions/runs/278383781

Ideally we should also add one test run with the split enabled, and even better, also add one tests/test_cmake_build/ test with the split enabled.

But yeah, CMake 😨 😨 😨

Let me know how you want to proceed.

[cirosantilli2]

Ping 🖖

[henryiii]

Sorry for the delay, this has been and will be on hold for a bit while we work on getting 2.6.0 out. Once that is out, then hopefully several of us will have more time to help with this. We should be able to do a test with and a test without, it should be easy to turn on and off for testing.

[cirosantilli2]

Ahh, thanks, I didn't know about the 2.6.0 release coming, I'll wait for that then.

[cirosantilli2]

Ping while waiting for a compilation :-)

[cirosantilli2]

Happy new year ping! 🥳 🥳 🥳 🥳 🥳 🥳 🥳

[YannickJadoul]

Happy new year, @cirosantilli2! :-)

I'll ping along ;-) @henryiii, what about picking this up once 2.6.2 has been released? Is there anything stopping us from pushing this into 2.7.0?

[henryiii]

We can make it 2.7.0 or 2.8.0. Personally I'm going to prioritize CLI11's transition first, and then hopefully learn from what I find there. But that shouldn't stop anyone else from working on this!

[henryiii]

For context, the 2.6.2 release should happen "soon"; after that I think the codebase is fairly open, except for maybe a small handful of waiting PR?

[YannickJadoul]

There will always be some PR waiting, but after running tests again and again and again, in the context of that Valgrind PR, I'm yearning for a faster build!

[cirosantilli2]

Oooops, I had made some email setting changes, and didn't get notified after my ping! Should be fixed now.

When the time arrives, just say NOW, and I'll jump on it and rebase 💯

[gabemblack]

Hi! My name is Gabe, and I'm a main developer/maintainer on the gem5 project. We are anxiously (but also patiently!) waiting for this change to go in, since it will be a major improvement in our build overhead for our tooling, etc, and also a major quality of life improvement for people who have to build our project often, like myself :-).

[giactra]

Friendly ping :) May I ask if there is a timeline for this PR to get accepted?

[henryiii]

We will be having a maintainers meeting tomorrow and this is one of our talking points. :)

So hopefully will know more after that.

[Skylion007]

Now that we have the Python 2 code removal and global clang-format PRs out of the way, we are interested in tackling both runtime and compilation performance of pybind11 and are happy to discuss compilation performance improvements such as this.

[giactra]

This is great @Skylion007. Do you have a timeline? Let us know what we can do to help you in the process

[royjacobson]

@Skylion007 any update on a roadmap for this? I'm also interested in getting this to work and could help if needed.

[Skylion007]

@royjacobson We would welcome a new version of this PR. The main issue is that it's gotten way too stale and needs to be updated. There would also likely have to be a similar PR to address the smart holder branch as well.

[Skylion007]

Also ping @giactra if you are interested in helping out.

[royjacobson]

Sounds great! I would like to get the CMake + doxygen changes committed with one sample TU to make sure they work, and then proceed to split the remaining headers in subsequent PRs.
About the smart holder branch - It seems to have diverged for quite some time and I wouldn't want to make merging more difficult or the splitting work twice. So if it is going to be merged before the next release I could maybe just make the cmake changes for now and wait with the splitting for the smart holder merge?

[giactra]

@Skylion007 @royjacobson Yes happy to help.