Would you consider merging a patch that optionally splits pybind11 into declarations vs definitions in a backwards compatible opt-in manner?

[cirosantilli2]

Edit: PR at: #2445

Hello!

TL;DR: a split into declarations/definitions would save a lot of time/money for the gem5 project which I work on. I already have a prototype which reduces build time by 40% from 25 minutes to 15 minutes.

If I were to send a patch that does this split, in an opt-in way that does not affect existing users at all, would you consider merging it?

I would like to know if this is reasonable before I sink time into something that won't get reviewed.

The proposed implementation is: I would just check if a macro if defined before inclusion of pybind11.hpp, say PYBIND11_HPP.

If the macro is defined, only declarations will be enabled, and therefore you won't get object redefinitions. So usage would be in a project that wants to opt-in would be like this.

In one single file of the project which gets linked at the end:

pybind11.cpp

#include <pybind11/pybind11.h>

This will be the only file to contain definitions of non-templated stuff that does not need to be defined in headers.

Then, on every other file of the project, include as:

#define PYBIND11_HPP 1
#include <pybind11/pybind11.h>

which will only expose declarations and not definitions, therefore preventing a ton of object redefinitions.

More details about the gem5 project can be found at: #708 (comment) maybe this issue is a duplicate of that one, but I'm not sure, feel free to close if you think yes. I'm just waiting for another long build and decided to annoy you a bit :-) If you promise to review, I will send a patch.

[virtuald]

FWIW, the way that I deal with pybind11's long build times in dev is by using ccache. Of course, that's less useful in CI.

How ugly would the resulting code be?

[cirosantilli2]

Hi Dustin!

Yes ccache can help, and I already use it locally, and could be in theory setup for CI, but I still feel that the gains of the split would be significant. I just need to check out random branches/rebase on master in separate worktrees too often.

About code ugliness, for every file I would split all definitions that don't need to be in the header (basically non-templates) into declaration + definition, and then group all definitions together at the bottom of each header file, and surround them with a single ifdef. So "not too ugly" I believe.

https://gem5-review.googlesource.com/c/public/gem5/+/29132 shows a hasty prototype that physically splits into .hpp and .cpp. I would basically take that and put the .cpp back inside the corresponding .hpp in an ifdef.

[virtuald]

I would love a 40% speedup. I definitely think it should be opt-in, since one of the reasons pybind11 is so easy to use is the fact that it's header-only. However, for users with large projects this seems great. I also maintain a fork of pybind11 that I include in my robotpy-build tool, which includes patches that haven't yet been merged upstream. If this weren't accepted upstream, I would potentially be interested in using such a fork.

Are the resulting libraries smaller or bigger?

It seems if one did this, would want to modify the existing CI to build/test both the split/unsplit version. I suppose looking at the build times in CI would be a proof that the speedup is real?

A project that I'm mildly involved with uses a .h/.inl paradigm for making the header files less messy, but perhaps it would be a good approach for this?

• Header only has declarations
• Inline file contains definitions

I like this because it makes the headers easier to read, and the split feels natural. For this, you could use a similar approach at the end of each file, instead of having the definitions there?

class options {
... 
};

#ifndef PYBIND11_HPP
  #include "options.hpp.inl"
#endif

I don't have any actual say about merging anything here, but I think this is a great idea if there aren't downsides and it results in real speedups.

[cirosantilli2]

Are the resulting libraries smaller or bigger?

I didn't note that when I last tested, I'd need to build again to check, but I don't see how they can possibly be larger since there will be less object redefinition flying around, if anything they would be smaller.

It seems if one did this, would want to modify the existing CI to build/test both the split/unsplit version. I suppose looking at the build times in CI would be a proof that the speedup is real?

It would be great to see on this pybind11 CI too. I'll patch whatever tests people want here. I wonder if you can see it on very small examples though: you'd need a certain number of object redefinitions to notice it. But I can guarantee that I did a gem5 build before and after that change alone, under the exact same conditions otherwise, and the build time fell by 40% (the project has several dozens of small autogenerated files that include pybind11).

[YannickJadoul]

Hi @cirosantilli2. We (i.e., the new group of maintainers that are currently trying to clean up the backlog of issues and PRs, etc) were discussing this yesterday, but somehow failed to actually post our conclusion here. But, here we go:

This seems quite interesting, indeed, a 40% speedup, though it probably depends on the amount of translation units you are compiling? To me, it's especially surprising because I would've guessed all the template instantiations are what make things slow?

https://gem5-review.googlesource.com/c/public/gem5/+/29132 shows a hasty prototype that physically splits into .hpp and .cpp. I would basically take that and put the .cpp back inside the corresponding .hpp in an ifdef.

This is already the first important step: the possibility to seen and play around with such a prototype.

From the project's perspective, there are two main concerns:

1. Maintainability is key (not exactly the same, but maybe related to @virtuald's "ugliness"?). pybind11 has the tendency to grow and become more complex in every direction, so we'd need to make sure this doesn't make things harder to add/maintain/be contributed. Duplication of code is also out of the question, of course.
2. pybind11 is advertised as header-only, which seems to be an important argument for adoption. Of course, your proposal still allows for this, but this is one of the things where we need to know if @wjakob wants to consider going there, as it does change/influence future directions of the library.

Related to the maintainability is how this fits into the build system and project structure. C++ sources should probably be in a src/ directory; do you have a plan or proposal for that? The good news is of course that we wouldn't be alone, as e.g. {fmt} has a similar approach.

The other thing is that we are still in the process of cleaning up old PRs and old issues. So if @wjakob doesn't veto this and we make things work work, be aware that it will take some time before things get merged and a new minor release gets created.

But yes, there is some interest, and while we currently can't promise anything before hearing from @wjakob, anything that can make this plan more concrete is welcome! :-)

[YannickJadoul]

The proposed implementation is: I would just check if a macro if defined before inclusion of pybind11.hpp, say PYBIND11_HPP.

My personal 2 cents and bike shedding: I'm not a big fan of that name. But that's of course some future discussion ;-)

[cirosantilli2]

Hi @cirosantilli2. We (i.e., the new group of maintainers that are currently trying to clean up the backlog of issues and PRs, etc) were discussing this yesterday, but somehow failed to actually post our conclusion here. But, here we go:

Fantastic news! Is the discussion public?

This seems quite interesting, indeed, a 40% speedup, though it probably depends on the amount of translation units you are compiling?

Absolutely. The majority of small projects out there will not benefit at all from this.

But potentially some "big and important" ones will. gem5 is more or less in that category I'd say. But see also #708 (comment) quote on pytorch saying pybind11 is too slow for them.

As mentioned at #708 (comment) "our build has 1271 object files, and 365 of them are simple auto-generated C++ files that include pybind11 to contain parameters for different C++ classes, and those 365 files dominate build times."

To me, it's especially surprising because I would've guessed all the template instantiations are what make things slow?

I was surprised too. I only spent 4 hours splitting headers for the prototype after I analyzed the project with the methodology from https://stackoverflow.com/questions/2351148/explicit-template-instantiation-when-is-it-used/59614755#59614755 and it clearly pointed to pybind11 as being about 50% of the unlinked object sizes. I also had the list of top object sizes per object, but didn't write it down, I'll do it next time.

https://gem5-review.googlesource.com/c/public/gem5/+/29132 shows a hasty prototype that physically splits into .hpp and .cpp. I would basically take that and put the .cpp back inside the corresponding .hpp in an ifdef.

This is already the first important step: the possibility to seen and play around with such a prototype.

From the project's perspective, there are two main concerns:

1. Maintainability is key (not exactly the same, but maybe related to @virtuald's "ugliness"?). pybind11 has the tendency to grow and become more complex in every direction, so we'd need to make sure this doesn't make things harder to add/maintain/be contributed. Duplication of code is also out of the question, of course.

Yes, this would add a "split definitions into header/c++ overhead" like other fast compiling c++ projects need to do :-)

2. pybind11 is advertised as header-only, which seems to be an important argument for adoption. Of course, your proposal still allows for this, but this is one of the things where we need to know if @wjakob wants to consider going there, as it does change/influence future directions of the library.

Related to the maintainability is how this fits into the build system and project structure. C++ sources should probably be in a src/ directory; do you have a plan or proposal for that? The good news is of course that we wouldn't be alone, as e.g. {fmt} has a similar approach.

Let me know what setup you think is best and I'll do it. src/ sounds fine. I would just keep things in the same header file, but I don't mind.

The other thing is that we are still in the process of cleaning up old PRs and old issues. So if @wjakob doesn't veto this and we make things work work, be aware that it will take some time before things get merged and a new minor release gets created.

No problem at all. I intend to continue working on gem5 for the foreseeable future :-)

But yes, there is some interest, and while we currently can't promise anything before hearing from @wjakob, anything that can make this plan more concrete is welcome! :-)

My personal 2 cents and bike shedding: I'm not a big fan of that name. But that's of course some future discussion ;-)

No problem, you may call it WUBBA_LUBBA_DUB_DUB for all I care 😆

[wjakob]

Hi @cirosantilli2,

thank you for bringing up this point, it's definitely worth discussing it. In principle, separate linking of the .cpp portion could even be done by Cmake (pybind11_module), so many users would automatically benefit.

That said, before considering to merge such a change, it would be important to discuss what the change you mention implies (i.e. what truly needs to be moved). I imagine that it's mainly the initialization-related parts like creation of the 'internals' data structure and the method dispatcher that could benefit from being moved into a .cpp file. Most of the rest is templated and thus not admissible, and the benefit of moving methods of simple wrapper classes like py::list is perhaps not as clear due to the resulting indirect costs (relatively code size small gain, making code less readable, preventing inlining of these methods, etc.)

I would also like to see this cited 40% reduction on a benchmark that I can try on my end. Part of the problem here is that we currently don't have infrastructure to monitor build time and per-commit changes.

Finally, there is another important metric that I care about much more than build time, because that's an area where we are not doing super well at the moment: binary size of the resulting bindings. This aspect needs to be part of the discussion as well.

Thanks,
Wenzel

[cirosantilli2]

Hi @cirosantilli2,

thank you for bringing up this point, it's definitely worth discussing it. In principle, separate linking of the .cpp portion could even be done by Cmake (pybind11_module), so many users would automatically benefit.

Fantastic, that would be even better.

That said, before considering to merge such a change, it would be important to discuss what the change you mention implies (i.e. what truly needs to be moved). I imagine that it's mainly the initialization-related parts like creation of the 'internals' data structure and the method dispatcher that could benefit from being moved into a .cpp file. Most of the rest is templated and thus not admissible,

Yes, templates simply cannot be moved.

At https://gem5-review.googlesource.com/c/public/gem5/+/29132 I just tried to (hastily) move everything that can be moved.

and the benefit of moving methods of simple wrapper classes like py::list is perhaps not as clear due to the resulting indirect costs (relatively code size small gain, making code less readable, preventing inlining of these methods, etc.)

I'll leave the criteria/benchmark selection to you, and I'll follow criteria/use benchmarks.

gem5 is not a good perf benchmark because the pybind11 is just a very shallow initialization wrapper.

I suspect this is the case for most projects, and people won't notice much slowdown due to removed inlining.

Not splitting everything has the downside that you will eternally need to correct people in pull requests :-)

Also worth noting that -flto can automatically inline functions these days: https://stackoverflow.com/questions/7046547/link-time-optimization-and-inline/61367211#61367211

I would also like to see this cited 40% reduction on a benchmark that I can try on my end. Part of the problem here is that we currently don't have infrastructure to monitor build time and per-commit changes.

Definitely, it would be amazing if others can reproduce, on Ubuntu 19.10 you can try:

sudo apt install libgoogle-perftools-dev libboost-all-dev m4 protobuf-compiler python-dev python-pip scons zlib1g-dev

git clone https://github.com/cirosantilli/gem5
cd gem5

# My prototype
git checkout pybind11-cpp-squash-2

# Time the build.
time scons --ignore-style -j `nproc` build/ARM/gem5.opt 

# One commit before my prototype on mainline gem5
git checkout HEAD~

mv build build-fast

time scons --ignore-style -j `nproc` build/ARM/gem5.opt 

Let me know if there are any problems, I might have missed some dependency.

Quoted 40% on my 8 core Lenovo ThinkPad P51 Ubuntu 20.: https://github.com/cirosantilli/notes/blob/61919a4c1d789eee3461b4c6011b6bbfb9118c28/my-hardware.adoc#thinkpad-p51 using SSD.

Finally, there is another important metric that I care about much more than build time, because that's an area where we are not doing super well at the moment: binary size of the resulting bindings. This aspect needs to be part of the discussion as well.

I'll check this out and get back to you.

Thanks,
Wenzel

[cirosantilli2]

I got a build error, let me investigate before others try it out.

[YannickJadoul]

Fantastic news! Is the discussion public?

No, it was a pretty informal "can we/how do we proceed with this?"

Absolutely. The majority of small projects out there will not benefit at all from this.

Actually, if some things from pybind11 don't need to be recompiled during development, that would still be nice. But OK, I understand.

Definitely, it would be amazing if others can reproduce, on Ubuntu 19.10 you can try:

To make it easier to try out, is there any chance you could turn this code into a PR, here? It doesn't need to be polished, but I think it might make things more concrete to move forward. That would make it easier to review as well. You do have our word we'll try to move forward on this and investigate the possibility (even though it might be slow progress, for now). If you don't have time or it's a lot more work to create a PR, that's fine as well, and we'll make do with the other change set, as well.

No problem, you may call it WUBBA_LUBBA_DUB_DUB for all I care

+1 for that name ;-)

[cirosantilli2]

OK, I remember now, gerrit didn't like my patch because it was too large and I made some mess there, now pushed the working branch to GitHub, updated the above comment instructions.

On Ubuntu 20.04 I've tested just now, I sometimes saw an improvement of only 30% not 40%. But the build time varies a lot, so it's hard to be certain. I think watching YouTube videos might have something to do with it:

First old:

real    25m38.292s
user    182m43.304s
sys     6m26.731s

Second old:

real    35m16.948s
user    229m43.098s
sys     8m49.819s

First new

real    17m22.439s
user    124m33.609s
sys     5m44.999s

Second new

real    18m34.282s
user    123m47.526s
sys     5m41.309s

Edit: I tried again when not watching YouTube videos, and with my computer on a table that does not overheat instead of on my bed, and I got results more like the original (37%) speedup:

real    24m4.998s
user    178m30.085s
sys     6m10.131s


real    15m11.539s
user    107m29.313s
sys     4m48.169s

@wjakob

Finally, there is another important metric that I care about much more than build time, because that's an area where we are not doing super well at the moment: binary size of the resulting bindings. This aspect needs to be part of the discussion as well.

I checked now, the build-fast/ARM/gem5.opt final executable fell from 1.8G to 962M unstripped (uses -O2 -g), God knows why that executable is so large. Manually stripped with strip fell from 51M to 47M. With strip --strip-debug it falls from 61M to 58M, so it does seem that the humongous bulk was actually from debug info and not some gem5 madness.

@YannickJadoul

To make it easier to try out, is there any chance you could turn this code into a PR, here? It doesn't need to be polished, but I think it might make things more concrete to move forward. That would make it easier to review as well. You do have our word we'll try to move forward on this and investigate the possibility (even though it might be slow progress, for now). If you don't have time or it's a lot more work to create a PR, that's fine as well, and we'll make do with the other change set, as well.

Great, I'm convinced. Let me just get clearance with our legal (maybe I should have done that already 😜), it will be really simple since nothing sensitive in this project, and then I'll post something here.

[cirosantilli2]

OK, legal person was on holiday, but everything is OK now, I'm giving it a shot. There are some functions that are marked as both PYBIND11_NOINLINE inline, is there a reason for this or just a bug?

[YannickJadoul]

Great! Curious to see how things will look :)

There are some functions that are marked as both PYBIND11_NOINLINE inline, is there a reason for this or just a bug?

As far as I know:

• PYBIND11_NOINLINE means "Please compiler, make this a separate function, not inline, and don't blow up the binary size of pybind11 libraries."
• And inline means "Don't bother with the One Definition Rule, though! This is in a header and thus I guarantee this is the same in every translation unit you'll compile."

[cirosantilli2]

OK. I'm asking because when I try to split I get warnings, e.g.:

main.cpp

__attribute__ ((noinline)) inline int myfunc();

int myfunc() { return 2; }

and:

g++ -c -ggdb3 -O0 -std=c++11 -Wall -Wextra -pedantic -o main.o main.cpp

gives:

main.cpp:3:39: warning: inline function ‘int myfunc()’ given attribute noinline
    3 | __attribute__ ((noinline)) inline int myfunc();
      |

I get the sneaky suspicion that __attribute__ is ignored in definitions (and therefore everywhere in the existing code before my patch) which is why the warning does not happen.

Does anyone know the correct placement of those?

I believe inline has two effects: the hide linkage, but also heuristic suggestion of inlining. static should hide linkage only, but then it gives unused warnings on every includer that does not use it.