Skip to content

vdb_tool: move implementations out of headers into a compiled vdb_tool_common library #2278

Description

@harrism

Summary

vdb_tool is implemented almost entirely in headers: Tool.h, Geometry.h, Parser.h, Calculator.h and Util.h under openvdb_cmd/vdb_tool/include/ total ~10,800 lines, while the two .cpp files (src/main.cpp, src/unittest.cpp) are thin drivers. vdb_tool_common is declared as an INTERFACE library, so nothing is separately compiled.

Tool.h alone is 4,201 lines. This works today, but it has three costs worth addressing before the tool grows further. Nothing is currently broken and this is not urgent.

Problem 1: the headers are not include-safe

class Tool is declared with a declaration-only body, and its members are then defined out-of-line at namespace scope without inline — currently 61 non-template member definitions in Tool.h alone (plus 4 template members, which are fine). Geometry.h and Parser.h follow the same pattern.

That is legal only because Tool.h is included by exactly one translation unit per executable:

  • src/main.cppadd_executable(vdb_tool src/main.cpp)
  • src/unittest.cppadd_executable(vdb_tool_test src/unittest.cpp)

Since vdb_tool_common is INTERFACE, it contributes no compiled TU, so each binary links exactly one definition of each member and the ODR is satisfied.

The moment anyone adds a second .cpp to either target — splitting unittest.cpp, adding a new entry point, adding a benchmark — every one of those 61 members becomes a duplicate-symbol link error. The failure is loud rather than silent, so this is a maintenance trap rather than a latent correctness bug, but it makes the headers effectively un-reusable and it is a surprising constraint that isn't documented anywhere.

The free helper functions in Tool.h (printBanner, formatBytes, formatNodes, etc.) are correctly marked inline, and there are no non-inline namespace-scope mutable globals in any of the five headers, so the problem is confined to the out-of-line member definitions.

Problem 2: the same ~75s of compilation is performed twice

Measured on this machine (g++ 13.3.0, -std=c++17 -O2, x86_64, headers from master, optional features NanoVDB/USD/OpenEXR/PDAL/Alembic all OFF), compiling each TU to an object file:

Translation unit Time
src/main.cpp 77.8s
src/unittest.cpp 89.0s
clean-build CPU total 166.8s

Decomposing where that goes:

Probe TU Time
#include <openvdb/openvdb.h> alone 2.1s
all 37 unconditional <openvdb/...> headers that Tool.h pulls in 5.2s
#include "Util.h" 2.4s
#include "Calculator.h" 2.5s
#include "Geometry.h" 4.0s
#include "Parser.h" 6.3s
#include "Tool.h" + empty main() 78.5s

Two things follow.

The OpenVDB include graph is not the problem. All 37 <openvdb/...> headers together cost 5.2s — 7% of Tool.h's 78.5s. Simply relocating includes out of the headers would save almost nothing. (I had initially assumed the opposite; the measurement says otherwise.)

main.cpp is Tool.h. A TU containing nothing but #include "Tool.h" and an empty main() costs 78.5s, versus 77.8s for the real main.cpp — the driver's own code is free. Likewise unittest.cpp's ~3,100 lines of tests account for only ~10.5s of its 89.0s. So ~75s of identical work — parsing and instantiating Tool.h's 61 member definitions and the OpenVDB templates they expand — is done twice per clean build, once for each executable.

For completeness, that ~75s is inherent to what the code does rather than one pathological spot:

Variant Time
Tool.h at -O0 / -O1 / -O2 36.1s / 45.1s / 77.8s
Tool.h at -O2 with the body of Tool::init() (675 lines) removed 73.9s

So it splits roughly 36s front-end / 42s optimizer, and the 675-line Tool::init() action table is only ~3.7s of it. The cost is spread across the ~60 action implementations, each instantiating heavy OpenVDB templates (meshToVolume, volumeToMesh, levelSetRebuild, FastSweeping, LevelSetAdvect, …) over several grid types. It can't be optimized away — but it can be stopped from happening twice, and it can be spread across TUs so it parallelizes.

Problem 3: Tool.h is 4,200 lines in a single file

Tool.h is 4,201 lines and contains the entire implementation of every action: the Tool class declaration, 61 out-of-line member definitions, 4 template members, and a 675-line Tool::init() that registers the full action table in one function. It grows with every feature — #2259 alone adds 337 lines.

Independently of compile time, that size has ordinary maintenance costs:

  • Unrelated work collides. Any two PRs adding actions touch the same file, usually near the same Tool::init() block, so merge conflicts are routine rather than exceptional.
  • There is no locality. Level-set ops, mesh conversion, point scattering, file I/O, rendering and the config parser are interleaved in one file with no structural boundary, so there's nothing to read short of the whole thing.
  • Blame and review are coarse. git log on Tool.h is the changelog for the entire tool, and a reviewer diffing it gets no signal about which subsystem changed.
  • The declaration/definition split is already implicit. The class body (lines 130–404) is effectively a header and everything after it is effectively a source file — the file is already structured as two files, just without the file boundary.

The fix below addresses this at the same time as Problems 1 and 2, which is the main argument for doing the split rather than the minimal inline fix.

Proposed fix

Convert vdb_tool_common from an INTERFACE library to a real (static) library and move the out-of-line definitions into .cc files:

openvdb_cmd/vdb_tool/
  include/  Tool.h  Geometry.h  Parser.h  Calculator.h  Util.h   # declarations + templates + inline helpers
  src/      Tool.cc Geometry.cc Parser.cc Calculator.cc          # the out-of-line definitions
            main.cpp unittest.cpp                                # drivers, unchanged

This fixes Problem 1 outright, and matches how the other tools in openvdb_cmd/ are structured.

Measuring a proxy for the resulting slim header (the OpenVDB include graph plus Parser.h and Geometry.h) gives 11.3s — and that is an upper bound, since those two headers would themselves be slimmed. Projected clean build:

now after
CPU total 166.8s ~111s (−33%)
wall clock at -j2 ~89s ~78s (−12%)
rebuild after editing a test ~89s ~22s (−75%)
rebuild after editing one action ~89s one .cc only

The wall-clock gain on a clean parallel build is modest, because a single Tool.cc still dominates. If Tool.cc is further split by action family (I/O, level-set ops, mesh conversion, point ops, rendering), that ~75s parallelizes and clean-build wall clock drops to roughly 25–30s. The incremental-rebuild gain is the one developers would feel day to day: today, touching one line anywhere in Tool.h rebuilds both executables from scratch.

Minimal alternative

If splitting the files is judged too invasive, marking the out-of-line member definitions inline fixes Problem 1 on its own and is a mechanical, low-risk change. It does nothing for Problems 2 or 3.

Caveats on the measurements

Timings are single-run on one machine and are meant to establish relative magnitudes, not absolutes. To compile against master's headers without a full configure I hand-generated openvdb/version.h from version.h.in with explicit template instantiation ON (the project default) and BLOSC/ZLIB/delayed-loading enabled. All VDB_TOOL_USE_* optional features were off; enabling NanoVDB in particular would raise the absolute numbers, and would raise the duplicated portion along with them.

Additional context

Noticed while reviewing #2259, which adds five more actions (-copy, -rename, -diagnose, -stats, -swap) in the existing style. That PR is consistent with the surrounding code and this issue is not a criticism of it — it just makes the trend more visible.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions