RobotLocomotion / drake

Model-based design and verification for robotics.
https://drake.mit.edu
Other
3.27k stars 1.26k forks source link

Bindings for drake::geometry have gotten huge #15858

Closed SeanCurtis-TRI closed 2 years ago

SeanCurtis-TRI commented 3 years ago

The file drake/bindings/pydrake/geometry_py.cc has gotten massive. So large that one user reported a 25 minute compile time. In addition, different aspects of geometry have different dependencies, and right now to compile a change in just about anything drags in dependencies and compilation for everything.

We'll follow the example given by drake/bindings/pydrake/systems/framework_py.cc and decompose the bindings into a collection of .cc files (each dedicated to some portion of the whole. In addition, we'll likewise partition the unit tests so that each binding compilation unit has a corresponding unit test (again, to facilitate more localized changes/tests).

Ultimately, we want to do this in such a way that the history of the code is preserved. So, we'll break the code out in multiple PRs. Each PR will have a commit sequence:

We'll have the following divisions will be added (one PR each, the order may be different than is shown below):

(As a footnote: due to an author SNAFU on #15860, we lost history on geometry_py.cc and geometry_test.py. #15879 sought to patch that failing by replacing geometry_py.cc with geometry_py_all.cc (and similarly for test). The patch was imperfect, but sufficient.)

jwnimmer-tri commented 3 years ago

I support this issue because the latency of any single compilation unit is important (e.g., the 25 minutes you cite above). The latency also relates to memory usage, so this will also help the build when RAM is scarce. Also because IDEs (and people!) struggle with the super long files.

... to compile a change in just about anything drags in dependencies and compilation for everything.

Note, however, that splitting up the bindings cc file will not really change how much gets rebuilt when a drake header changes, nor how much gets relinked when a drake cc file changes. Because pydrake eats all of drake's headers and object code as a single shared library dependency, any change to drake will always re-fire most all of the pydrake build actions. With this fix, though, there will be more concurrency available, so developers with lots of CPUs will see a nice reduction in long-tail build latency.

SeanCurtis-TRI commented 2 years ago

On the splitting, there is concrete value beyond latency. (Although my earlier language was imprecise and implied sweeping benefits), at the very least, the decomposition means that some tests don't require MultibodyPlant bindings. Others don't require mathematical program, sensor, or analysis bindings. So, if one is working in the corner, one can work without being touched by high-latency binding code. Not as good at leaving the corresponding portions of Drake uncompiled, but I'll take everything I can get.

jwnimmer-tri commented 2 years ago

Ah yes, that's accurate -- intra-pydrake dependencies are open to improvement, and in particular multibody_tree_py.cc is one of the high-latency cc files that defeats parallelism and also really needs to be split up, similar to how geometry_py is being split up here.

SeanCurtis-TRI commented 2 years ago

I bet you can guess how eager I would be to tackle the multibody_tree_py.cc break up. ;)

jwnimmer-tri commented 2 years ago

I believe the final two PRs here have already been merged.