pybind11
pybind11 copied to clipboard
pybind
Split pybind11 into headers and cpp files to speedup compilation
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 https://github.com/pybind/pybind11/issues/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
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).
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.
Sounds good to me! That should solve the circular dependency problem and the src vs. include directory thing.
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.
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! :laughing: 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 :laughing: 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).
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.
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.
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.
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.
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 :-)
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?
I'm still in favor of
-inl.hbecause: a) having a .cpp file in/includeand shipped with your library in packaging is odd, b) compilers sometimes complain if you put an include guard on a.cppfile (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 :+1:
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 :grin:
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.
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.
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.
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.
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.
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?
OK, I split the second unrelated commit to https://github.com/pybind/pybind11/pull/2476 now.
Also thanks for the actions tip, I was not very familiar with how they work.
When checked the actions, the workflows list is empty for some reason.
The I manually created a dummy one, and they appeared.
Then I force pushed the new workflow away, and tried a CI run at https://github.com/cirosantilli2/pybind11/actions/runs/247775849 but all tasks are failing with Check failure on line 1 in .github and I'm not sure what that means.
I'm assuming this would be a 3.0 level feature?
Does it have to be, though? In principle, the changes are non-intrusive, and could be part of a minor release?
I updated the .cpp files in src now.
@henryiii can you check if it is easy for you to fix the cmake --build build --target test_cmake_build failure mentioned at? https://github.com/pybind/pybind11/pull/2445#issuecomment-690081371
If you don't get the time, I'll try myself more later on.
The other thing needed would be to add this to the default testing system I think. Once again, if this is easy to anyone, give it a try. We'd also need to decide if both tests (header only + .cpp version) would run always or not.
Let me know if there's anything more needed from my side to move this forward :+1:
Once this is well approved, I'll do a final rebase.
Thanks @henryiii !! I've squashed your change in as well now.
Now that those failures are gone, I fixed some others that became more visible on the last force push:
- missing PYINBD11_INLINE on a macos ifdef area
- skip embed.cpp compilation in pypy in CMakeFiles.txt
Now there is only one more problem in the tests: Doxygen. If I remove the doc build, all other tests passed as shown at: https://github.com/cirosantilli2/pybind11/actions/runs/266615314
The doc build failure can be seen at: https://github.com/cirosantilli2/pybind11/runs/1148460736?check_suite_focus=true
First, I already had to modify EXPAND_ONLY_PREDEF = NO in Doxyfile otherwise it gets messed up with PYINBD11_INLINE and fails earlier.
And after that it fails with:
Warning, treated as error:
/__w/pybind11/pybind11/docs/reference.rst:27:doxygenclass: Cannot find class "object_api" in doxygen xml output for project "pybind11" from directory: .build/doxygenxml/
which happens due to the inclusion in docs/reference.rst:
.. doxygenclass:: object_api
:members:
If I remove that one in particular, all other doxygenclass also seem to fail, so it is not specific just for that one.
I checked, and the file docs/.build/doxygenxml/classobject__api.xml does exist.
Any ideas?
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 :fearful: :fearful: :fearful:
Let me know how you want to proceed.
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.
Ahh, thanks, I didn't know about the 2.6.0 release coming, I'll wait for that then.
Happy new year ping! :partying_face: :partying_face: :partying_face: :partying_face: :partying_face: :partying_face: :partying_face:
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?