[opt](build) Add ENABLE_UNITY_BUILD and pilot unity builds on three glue targets - #66712
Conversation
Unity builds merge groups of .cpp files into jumbo translation units for much faster full builds, but three workflows need them off: per-file diagnostics while iterating on a single source, per-file tooling (clang-tidy, coverage), and A/B isolation when debugging a unity-only failure. Introduce the knob ahead of the first unity targets (following commits) so every per-target UNITY_BUILD property is gated on it from day one: - be/CMakeLists.txt: option(ENABLE_UNITY_BUILD ON), reported via message() - build.sh: pass -DENABLE_UNITY_BUILD through (default ON, overridable from the environment / custom_env.sh, same pattern as ENABLE_PCH) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Gdfkk7RqgD5e3Uv7bTM3NV
The header opened with `#ifndef _SCHEMA_SCANNER_HELPER_H_` but never defined the macro (and one include sat outside the guard), so the guard never took effect. Harmless while every TU included it exactly once; it breaks immediately under a unity build. Use #pragma once like the other 50 headers in this directory. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The 51 schema-scanner TUs are homogeneous glue code whose compile cost is dominated by re-parsing the same header closure once per file. Merging them into a single unity TU (UNITY_BUILD_BATCH_SIZE 0) measured on clang 20 arm64, serial, cold file cache: - sum CPU with PCH: 124s -> 12.5s (9.9x); without PCH: 206s -> 14.1s (14.6x). Unity without PCH still beats individual TUs with PCH (8.8x). - archive symbol parity: all 2303 external defined symbols preserved (plus 5 harmless weak unique_ptr<SchemaXxxScanner> instantiations) - __text shrinks 1.07MB -> 0.50MB (linkonce_odr dedup) and libInformationSchema.a shrinks 334MB -> 29MB - exposed one latent bug: the broken include guard fixed in the previous commit The UNITY_BUILD property is gated on ENABLE_UNITY_BUILD (previous commit). A future file whose file-scope symbols clash inside the unity TU can opt out via SKIP_UNITY_BUILD_INCLUSION. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…Reader Preparation for merging these sources into unity TUs: - 27 http action files each carried a private copy of the same file-scope constants (HEADER_JSON x16, TABLET_ID x7, SCHEMA_HASH x4, OP/PATH/ TOKEN_PARAMETER/MEBIBYTE x2). Move them to one shared header, service/http/action/action_constants.h, as C++17 inline variables. Values are unchanged; two now-empty anonymous namespaces are removed. - prefix_query declared get_prefix_terms(IndexReader*) relying on a header-level CL_NS_USE(index) using-directive. Inside doris::segment_v2 that unqualified name flips to doris::segment_v2::IndexReader as soon as any sibling source brings that type into scope. Spell out lucene::index::IndexReader in the declaration and definition. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Second and third targets of the unity rollout (after InformationSchema): - Service: merge the ~58 http handler TUs into one unity TU. Non-http sources (service entry points, arrow_flight) and two macro-defining files (http_parser.cpp, be_thread_stack_action.cpp) stay individual. Unity TU compiles in 16s where the members summed to ~5.6min of slot time in the cold bench; archive keeps all 4054 external symbols. - Storage: merge the storage/index/ subtree (~111 TUs, CLucene-heavy closure) into four unity TUs of <=32 sources; the rest of Storage is explicitly opted out per file. Four macro-defining index files stay individual. Archive keeps all external symbols (8 weak linkonce_odr template instantiations dedup away) and shrinks 1492MB -> 1318MB. Both UNITY_BUILD properties are gated on ENABLE_UNITY_BUILD; the SKIP lists stay unconditional (inert without unity). contrib (openblas/clucene) was evaluated and intentionally left alone: the f2c-generated LAPACK sources and snowball stemmers define clashing file-scope statics, and contrib compiles once and rarely changes. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
run buildall |
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review status: capped/incomplete after the third and final review round. The final round surfaced two additional accepted tooling defects, so this should not be read as convergence.
I found four actionable build/configuration issues, attached inline: the OFF switch is dropped by the BE UT/UT-coverage configure path; native CMake unity can override the advertised custom OFF state; the default unity dependency shape is silently discarded by cut_impact.py; and compile-benchmark directory attribution is redirected into the build-tree bucket.
Checkpoint conclusions:
- Goal and proof: The default-ON pilot does form the intended current batches (52 InformationSchema, 58 service/http, and 101 storage/index sources), and completed Linux/macOS compile, BE UT, coverage, formatting, and regression checks are green. Those results prove the ordinary ON path, not the four deterministic negative/tooling paths reported here.
- Smallness and clarity: The source-level qualification, guard repair, and constant consolidation are focused. The configuration/tool integration is not complete enough for a default-on switch.
- Concurrency and lifecycle: No runtime thread, lock, shared mutable ownership, memory-accounting, or cleanup path changes. Exact unity batch order, macro/lookup leakage, archive grouping, and static initialization/destruction were reviewed; no additional defect was substantiated.
- Configuration and parallel paths: Normal build, clean/reused cache, BE UT, UT coverage, compile-bench, clang-tidy, native/custom unity controls, PCH, optional TLS/Variant, ANN, and platform paths were traced. The four comments are the unresolved implementation gaps; clang-tidy command inference was separately reproduced and the earlier silent-skip concern was dismissed.
- Compatibility and data correctness: No FE/BE protocol, callable ABI, storage format, persisted metadata, transaction, visibility, delete-bitmap, nullable-column, or rolling-upgrade behavior changes. The explicit CLucene type names the same type and preserves ownership/cleanup.
- Conditions, errors, and observability: Runtime status/exception behavior is unchanged. The build-tool failures are silent or misleading specifically where called out inline; runtime logs and metrics do not need changes.
- Tests and results: No runtime golden result is required for the source-hygiene changes, but negative fixtures are needed for clean UT OFF propagation, native-ON/custom-OFF precedence, unity-shaped Ninja dependencies, and unity-path report grouping.
- Performance: The reported clean-build speed/RSS evidence and bounded Storage batches are credible; no runtime regression was found. The benchmark attribution issue prevents its directory report from accurately explaining the optimized segments.
- Other/user focus: All 36 changed paths and every current unity sibling were reviewed. No additional user focus was supplied; the full PR was reviewed without further substantiated issues.
Because new valuable findings appeared in Round 3, this review is submitted as capped/incomplete under the review contract even though every current candidate has been accepted, deduplicated, or dismissed with evidence.
| -DSTRIP_DEBUG_INFO="${STRIP_DEBUG_INFO}" \ | ||
| -DDISPLAY_BUILD_TIME="${DISPLAY_BUILD_TIME}" \ | ||
| -DENABLE_PCH="${ENABLE_PCH}" \ | ||
| -DENABLE_UNITY_BUILD="${ENABLE_UNITY_BUILD:-ON}" \ |
There was a problem hiding this comment.
The new switch is only forwarded by build.sh. run-be-ut.sh has its own clean CMake configure (run-be-ut.sh:321-341) and does not pass this value, so ENABLE_UNITY_BUILD=OFF ./run-be-ut.sh --clean ... still recreates the cache with this option's default ON. That means the advertised per-file escape hatch cannot be used for the standard BE UT/UT-coverage workflow when diagnosing a unity-only failure. Please mirror this plumbing and effective-value logging in run-be-ut.sh.
There was a problem hiding this comment.
Fixed in 0dcc67c — run-be-ut.sh now forwards -DENABLE_UNITY_BUILD and reports the effective value next to ENABLE_PCH.
One correction to the report: the gap was wider than --clean. Since the script never passed the variable at all, the very first configure cached the option default ON and no later run could change it, so ENABLE_UNITY_BUILD=OFF ./run-be-ut.sh was inert with or without --clean.
| # Batch size 0 merges all sources into a single unity TU (~10x faster than | ||
| # compiling them individually). A new file whose file-scope symbols clash | ||
| # inside the unity TU can opt out via SKIP_UNITY_BUILD_INCLUSION. | ||
| if (ENABLE_UNITY_BUILD) |
There was a problem hiding this comment.
This false branch is not authoritative if the cache already has CMake's standard CMAKE_UNITY_BUILD=ON. That variable initializes each target's UNITY_BUILD property when add_library runs; with ENABLE_UNITY_BUILD=OFF, this block simply leaves the inherited ON value untouched. I reproduced ENABLE_UNITY_BUILD=OFF, CMAKE_UNITY_BUILD=ON, and UNITY_BUILD=ON, with a database containing only a generated unity source. The same positive-only gate appears in Service and Storage, so the advertised OFF escape hatch can fail on a reused native-unity cache. Please explicitly set the pilot targets' property OFF when this option is OFF.
There was a problem hiding this comment.
Fixed in e1299ba — agreed, and thanks for reproducing it. The option is now normalized to a strict ON/OFF once in be/CMakeLists.txt, and all three pilot targets set UNITY_BUILD ${DORIS_UNITY_BUILD} unconditionally, so OFF is an explicit OFF rather than an empty branch.
Verified on this tree with your exact repro, -DENABLE_UNITY_BUILD=OFF -DCMAKE_UNITY_BUILD=ON: 28 unity TUs across InformationSchema/Service/Storage before the change, 0 after. The ordinary -DENABLE_UNITY_BUILD=ON configure still produces exactly the 6 advertised batches (1 + 1 + 4).
Normalizing has a second benefit: an empty -DENABLE_UNITY_BUILD= forwarded by a script would otherwise reach set_target_properties as a missing value and fail the configure with an argument-count error.
| # Merge groups of .cpp files into jumbo translation units for much faster full | ||
| # builds. Turn OFF for precise per-file diagnostics, per-file tooling | ||
| # (clang-tidy/coverage), or the finest-grained incremental rebuilds. | ||
| option(ENABLE_UNITY_BUILD "Enable CMake unity builds for BE targets" ON) |
There was a problem hiding this comment.
Defaulting this option ON makes the standard compile-benchmark dependency model silently incomplete. cut_impact.py::load_tus() accepts a Ninja dependency block only when its first dependency is an original source under be/src or gensrc/build; a unity block starts with a generated .../Unity/unity_N_cxx.cxx, so it discards the whole block, including all later original-source/header dependencies. The real loader retained the standalone fixture but returned no TU for the equivalent unity shape. Consequently edge/audit/why omit all six pilot batches (211 sources). Please expand unity blocks or fail explicitly with a non-unity rebuild instruction.
There was a problem hiding this comment.
Fixed in 90d33da, taking the second of your two options: load_tus() now counts unity blocks and exits with the rebuild instruction (ENABLE_UNITY_BUILD=OFF ./build.sh --compile-bench) instead of returning a truncated TU set.
Expanding the block is not a safe alternative. A unity block carries the union of its members' closures, so attributing it back to each member would make every source look like it reaches every header any sibling pulls in — inflating both the affected-TU counts and the seeding advice that edge/audit emit. Silent under-coverage would be traded for silent over-estimation, and this tool exists to be precise about exactly that.
Fixture added: build-support/tests/test-compile-bench-unity.sh feeds load_tus() a canned ninja -t deps database in both shapes and asserts the standalone block is still kept and the unity block is refused with the instruction.
| # Merge groups of .cpp files into jumbo translation units for much faster full | ||
| # builds. Turn OFF for precise per-file diagnostics, per-file tooling | ||
| # (clang-tidy/coverage), or the finest-grained incremental rebuilds. | ||
| option(ENABLE_UNITY_BUILD "Enable CMake unity builds for BE targets" ON) |
There was a problem hiding this comment.
This default also breaks the compile benchmark's advertised per-directory attribution for every pilot batch. The timing wrapper records the generated source path (be/build_<Type>_compile_bench/src/<dir>/.../Unity/unity_N_cxx.cxx), but report.py::group_keys() recognizes logical modules only when a path starts with be/src. With the real helpers, an InformationSchema source groups under be/src/information_schema, while its unity source groups under be / be/build_Release_compile_bench. The report therefore hides the optimized segments' costs in a build-directory bucket. Please normalize unity paths to logical target/source directories and add a report fixture.
There was a problem hiding this comment.
Fixed in 90d33da — report.py maps a generated unity source back to its target's directory before grouping, so be/build_<Type>_compile_bench/src/storage/CMakeFiles/Storage.dir/Unity/unity_0_cxx.cxx now rolls up under be/src/storage instead of the build-tree bucket. The regex was checked against the paths a real configure generates for the six pilot batches.
One limit worth stating rather than hiding: attribution is only as fine as the target's own directory. The Service batch merges just be/src/service/http sources but lands on be/src/service, and the Storage batches land on be/src/storage rather than be/src/storage/index. A unity TU is one timing for all of its members by construction, so second-level attribution inside a batch is not recoverable from the timing data — only from an ENABLE_UNITY_BUILD=OFF run.
Fixture added in build-support/tests/test-compile-bench-unity.sh (unity path, ordinary source, and a non-unity build-tree path).
run-be-ut.sh runs its own cmake configure and did not forward the new switch, so the option default (ON) always won there: `ENABLE_UNITY_BUILD=OFF ./run-be-ut.sh` was silently ignored, with or without --clean, because the first configure caches ON and nothing ever overrides it. That removes the escape hatch exactly where it is needed most -- diagnosing a unity-only failure in the BE UT / UT-coverage build. Forward the value and report it, mirroring ENABLE_PCH. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Gdfkk7RqgD5e3Uv7bTM3NV
…TY_BUILD Each pilot target set its UNITY_BUILD property inside `if (ENABLE_UNITY_BUILD)`, which is a one-way gate: OFF took the empty branch and left the property alone. CMake's own CMAKE_UNITY_BUILD initializes UNITY_BUILD when add_library runs, so a cache carrying CMAKE_UNITY_BUILD=ON kept the pilot targets unity-built while the build advertised the opposite. Normalize the option to a strict ON/OFF once and set the property unconditionally from it, so OFF is an explicit OFF. Normalizing also keeps an empty -DENABLE_UNITY_BUILD= (an unset variable forwarded by a script) from turning into a malformed set_target_properties call. Verified on this tree with `-DENABLE_UNITY_BUILD=OFF -DCMAKE_UNITY_BUILD=ON`: 28 unity TUs across the three pilot targets before, 0 after; the ordinary `-DENABLE_UNITY_BUILD=ON` configure still produces exactly the 6 advertised batches (InformationSchema 1, Service 1, Storage 4). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Gdfkk7RqgD5e3Uv7bTM3NV
…n units Both compile-bench tools read `ninja -t deps` / the timing records of a build directory that, with unity builds on by default, no longer has one entry per source file. Neither noticed: - cut_impact.py accepted a dependency block only when its first dependency was an original source, so a block starting with a generated unity .cxx was dropped whole -- along with every original source and header listed after it. `edge`/`audit`/`why` then ran on a silently truncated TU set. It cannot be repaired by attributing the batch closure to each member: that union would make every member look like it reaches every header a sibling pulls in, inflating both the affected counts and the seeding advice. So refuse the build dir and say how to rebuild it. - report.py grouped by directory only for paths under be/src, so every unity batch landed in a be/build_<Type>_compile_bench bucket instead of the module it came from -- hiding the cost of exactly the segments unity optimizes. Map a unity source back to its target's directory before grouping. Both behaviours are invisible on a normal build (one is a refusal, the other a grouping key), so build-support/tests/test-compile-bench-unity.sh pins them with a canned unity deps database and a unity path fixture. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Gdfkk7RqgD5e3Uv7bTM3NV
rebuild_radius.py counts, per header, how many object targets depend on it. A unity build answers that question at batch granularity: the 51 InformationSchema scanners become one object, so a header they all include reports 1 dependent instead of 51. The number is correct for that configuration but not comparable with a per-file run, which is how the tool is normally used. Say so in the output when unity objects are present, rather than letting the count quietly change meaning. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Gdfkk7RqgD5e3Uv7bTM3NV
|
Pushed fixes for all four findings from the review above (each answered inline):
Plus 6bb5239, which was not in the review: Verification:
Two notes where the fix deliberately differs from the suggestion:
|
|
/review |
There was a problem hiding this comment.
Review status: converged in Round 3. All three final-round reviewers returned NO_NEW_VALUABLE_FINDINGS; every candidate was validated, deduplicated, accepted, or dismissed with concrete evidence.
I found two actionable P2 tooling issues, attached inline: compile-benchmark results do not persist/display the effective unity mode, and the natural-closure sweep can false-clean a source through earlier unity-batch members.
Checkpoint conclusions:
- Goal and scope: The PR's core goal is faster cold/full BE builds through default-on unity for three pilot targets with an authoritative OFF escape hatch. The normalized target properties, collision repairs, and build/UT propagation implement that core goal, but the two companion-tool paths above remain incomplete. The changes are otherwise focused.
- Correctness and lifecycle: All admitted InformationSchema, Service HTTP, and Storage/index sources were inventoried for macro/pragma leakage, lookup collisions, anonymous/header-internal state, static initialization/destruction, archive extraction, and configuration-specific admission. The shared HTTP constants preserve their values and immutable use, and the CLucene qualification preserves the intended type. No additional C++ runtime or lifecycle defect was substantiated.
- Concurrency: No Doris runtime threads, locks, ownership, transactions, persistence, or shared mutable data are changed. The build timing JSONL remains serialized by the existing
flock. - Configuration and parallel paths: Normal BE, BE UT, inherited native unity, PCH, compile-benchmark, clang-tidy, coverage, TU/syntax/closure probes, and Ninja dependency tools were traced. ON/OFF propagation is correct in normal and UT configure paths. The natural-closure path is not source-isolated, and archived benchmark configuration is not self-describing, as called out inline.
- Compatibility and data safety: There is no FE/BE protocol, storage format, persisted Doris metadata, transaction, data-write, or rolling-upgrade change. Runtime error/status behavior is unchanged.
- Conditions and observability: Current HTTP/parser/index skip rules and TLS, Variant, ANN, test, compiler, and macOS branches were checked. Configure output reports the effective option; persistent benchmark observability is the missing boundary in the first comment.
- Tests and evidence:
build-support/tests/test-compile-bench-unity.sh, relevant Bash syntax checks, Python syntax checks, andgit diff --checkpassed. Minimal CMake/Ninja probes reproduced the closure false clean and confirmed that removed unity targets are filtered after an ON-to-OFF reconfigure, which dismissed the stale-dependency candidate. No Doris build was run, as required by the review prompt. The added fixture does not cover either accepted issue. - Performance: The intended trade is fewer repeated header parses for coarser incremental/object granularity and larger TUs. Storage is bounded at 32 sources per batch, the two smaller targets intentionally use batch size 0, and OFF remains available. No additional concrete performance defect was found.
- User focus: No additional user-provided focus was supplied; all 41 changed paths and the full PR were reviewed.
| -DSTRIP_DEBUG_INFO="${STRIP_DEBUG_INFO}" \ | ||
| -DDISPLAY_BUILD_TIME="${DISPLAY_BUILD_TIME}" \ | ||
| -DENABLE_PCH="${ENABLE_PCH}" \ | ||
| -DENABLE_UNITY_BUILD="${ENABLE_UNITY_BUILD:-ON}" \ |
There was a problem hiding this comment.
[P2] Record unity mode in compile-benchmark metadata
This forwards a setting that changes the benchmark's TU population and timing identities, but compile_bench_init persists PCH/toolchain/generator/build type without the effective ENABLE_UNITY_BUILD, so summary.json and the human report cannot say whether an archived run used the generated pilot TUs or the original sources. The prior UT thread fixed configure propagation; it does not make benchmark results self-describing. Please persist and display the effective mode, and add an ON/OFF metadata fixture.
| # Merge groups of .cpp files into jumbo translation units for much faster full | ||
| # builds. Turn OFF for precise per-file diagnostics, per-file tooling | ||
| # (clang-tidy/coverage), or the finest-grained incremental rebuilds. | ||
| option(ENABLE_UNITY_BUILD "Enable CMake unity builds for BE targets" ON) |
There was a problem hiding this comment.
[P2] Keep the natural-closure sweep source-isolated
The earlier cut_impact.py fix now refuses this unity database, but closure-sweep.sh still runs syntax_sweep.py --no-pch, whose stated guarantee is to check each source's natural include closure. That tool accepts a generated unity file as one TU and compiles its ordered .cpp includes together, so declarations, headers, or macros from an earlier member can make a broken later member pass; baseline failures also collapse to a batch name. Please make this sweep refuse unity with the existing OFF rebuild instruction or replay every member independently, and add a masking-sibling fixture.
|
run buildall |
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
|
run buildall |
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
|
run performance |
github-actions
Bot
added
the
approved
|
PR approved by at least one committer and no changes requested. |
…per-db scanners (#66769) ### What problem does this PR solve? Related PR: #66732, #66712 Problem Summary: `schema_per_db_scanner.h` gives its `unique_ptr` member a default member initializer: ```cpp std::unique_ptr<Block> _fetched_block = nullptr; ``` That initializer makes gcc instantiate `~unique_ptr<Block>()` in **every** translation unit that includes the header, and the instantiation needs `Block` to be a complete type. None of the seven `SchemaPerDbScanner` subclasses include `core/block/block.h`, so each of them fails to compile with g++ 15: ``` In instantiation of 'void std::default_delete<_Tp>::operator()(_Tp*) const [with _Tp = doris::Block]': bits/unique_ptr.h:399:17: required from 'std::unique_ptr<_Tp, _Dp>::~unique_ptr() [with _Tp = doris::Block]' schema_per_db_scanner.h:68:45: required from here 68 | std::unique_ptr<Block> _fetched_block = nullptr; | ^~~~~~~ bits/unique_ptr.h:91:23: error: invalid application of 'sizeof' to incomplete type 'doris::Block' ``` Declaring the destructor out of line -- which the header already does -- does not help: the default member initializer is what marks `~unique_ptr<Block>()` as used. clang does not instantiate the destructor there, which is why only the gcc build reports it. `SchemaScanner::_data_block` has the same type and the same forward-declared `Block` and is fine, because it has no initializer. Dropping the `= nullptr` leaves the member null just the same, and costs nothing at build time -- the alternative, including `core/block/block.h` from the header, would pull that closure into seven more translation units. Note that the failure is currently masked on master: since #66712 turned `ENABLE_UNITY_BUILD` on by default, all of `information_schema` is merged into a single unity TU, and `schema_per_db_scanner.cpp` in that TU includes `core/block/block.h`, so `Block` ends up complete for its neighbours. Building with `ENABLE_UNITY_BUILD=OFF` still fails, as does any file that later opts out via `SKIP_UNITY_BUILD_INCLUSION`. ### Release note None ### Check List (For Author) - Test - [x] No need to test or manual test. Explain why: - [x] This is a refactor/code format and no logic has been changed. Verified as a build fix instead: reproduced the exact diagnostic with g++ 15.2 on a reduced case and confirmed it compiles clean after removing the initializer, then re-checked the real translation units (`schema_key_column_usage_scanner.cpp`, `schema_partitions_scanner.cpp`, `schema_table_options_scanner.cpp`, `schema_per_db_scanner.cpp`) against the edited header. - Behavior changed: - [x] No. - Does this need documentation? - [x] No. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Fourth target of the unity rollout (after InformationSchema, the http glue and storage/index in apache#66712). 167 of 174 Exec TUs join unity batches of <=12 sources (14 unity TUs), gated on the ENABLE_UNITY_BUILD knob so -DENABLE_UNITY_BUILD=OFF still compiles every TU individually. Opted out: five files whose file-scope macros must not leak into unity siblings, plus the two heaviest template-instantiation TUs (operator.cpp, hashjoin_build_sink.cpp) which would dominate any batch they join; scan_operator.cpp is both. Measured on the validation build (-j14 + PCH): the 14 unity TUs take 425s of slot time under full parallel load where their members summed to ~1418s in the cold bench (3.3x); the whole target drops from 1533s to 539s. The largest unity TU compiles in 32s / 2.6GB RSS standalone, below the existing per-TU peaks elsewhere in the build. The archive keeps all external symbols (three weak linkonce_odr template instantiations dedup away) and shrinks 2450MB -> 641MB. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ND7L1ZVTJf91TBpLwYSqct
> Part of the BE build-time optimization series tracked in #66715. > > Split out of **#66510, which carries the whole > BE build-time batch. #66712 introduced the `ENABLE_UNITY_BUILD` switch and piloted > unity builds on three low-risk glue targets. This PR extends unity to **Exec and > Exprs** — the two heaviest targets in the BE build and the largest single source of > the unity line's win. The remaining targets follow in one more PR. ### What problem does this PR solve? Related PR: #66510, #66712 Problem Summary: Same mechanism as #66712: most of the cold-build cost of glue-heavy targets is **re-parsing the shared header closure once per small `.cpp`**, and CMake's `UNITY_BUILD` makes a batch pay that parse once. What is new here is the scale — Exec (174 TUs, `libExec.a` 2450 MB) and Exprs (287 TUs, `libExprs.a` 2721 MB) are the two heaviest targets in the tree, and their glue shares the heaviest closures (`operator.h`/`dependency.h` for Exec; the vexpr/factory closure for Exprs). The four commits: 1. **Deduplicate exec file-scope names that clash under unity** (no behavior change): `file_scanner.cpp`/`file_scanner_v2.cpp` both defined the Iceberg delete content codes and `is_iceberg_position_deletes_sys_table()` in anonymous namespaces — the shared trio moves to `iceberg_scan_semantics.h` (`file_scanner_v2_test.cpp` carried a third copy, kept file-local by #66615 because this header move had not landed yet; it now uses the header too). `vtablet_writer.cpp`/`vtablet_writer_v2.cpp` both defined a file-scope `CLOSE_WAIT_EVENT_FALLBACK_MS` — scoped into `IndexChannel` and `VTabletWriterV2`; v2's file-scope `on_partitions_created()` trampoline renamed `on_partitions_created_v2` (the two functions cast to different writer types). `exchange_sink_operator.cpp`'s namespace-scope `timer_name` renamed `wait_for_dependency_timer_name` (shadowed unity siblings' locals under `-Wshadow -Werror`). 2. **Unity for the whole Exec target**: 167 of 174 TUs join 14 unity batches of ≤12 sources, gated on `ENABLE_UNITY_BUILD` like the pilot targets. Opted out: five files whose file-scope macros must not leak into siblings, plus the two heaviest template-instantiation TUs (`operator.cpp`, `hashjoin_build_sink.cpp`) which would dominate any batch they join; `scan_operator.cpp` is both. 3. **Three latent defects the Exprs conversion surfaced** (stand on their own): `dictionary_factory.h` had **no include guard at all** — any TU reaching it through two include paths fails with a class redefinition, and under unity the clang error recovery poisoned unrelated batch members with spurious `-Warray-bounds` diagnostics. Now `#pragma once`. And `function_dict_get_many.cpp` had copy-pasted the `DictGetState` struct from `function_dict_get.cpp` at namespace scope — renamed `DictGetManyState` so the two TUs can share a batch. And `function_variant_element_v2.cpp` kept `OwnedPathSegment` in an anonymous namespace while using it as a field of the externally-visible `ResolvedVariantElementV2Path::Impl` — gcc's `-Wsubobject-linkage` (`-Werror`) rejects exactly that once the file is `#include`d into a unity batch instead of being the main file of its TU (clang has no such warning); the struct moves to namespace scope. 4. **Unity for the Exprs glue**: 246 of 287 TUs join 31 unity batches of ≤8 sources, same switch. Opted out: the flex/bison/gperf generated tables, seven macro-leaking files, the 30 heavy template-instantiation TUs (>15 s wall or >2.2 GB RSS in the compile bench: the min_max/collect/topn/percentile aggregate family, `in.cpp`, `multiply.cpp`, `function_array_aggregation.cpp`, …) whose per-file codegen would only stack into jumbo poles — and three files that tests compile a second time by `#include`ing the `.cpp` (`function_variant_element.cpp`, `uuid.cpp`, `function_jsonb_transform.cpp`), see the verification section. ### Measured results All numbers from the development branch this series is split from, macOS arm64 + clang 20, `-j14`, `ENABLE_PCH=ON`, cold builds, back-to-back A/B. The baseline is the #66712 state of that branch (10m16s), so the two waves compose with the pilot: | wave | build phase wall | target slot time | archive size | |---|---|---|---| | Exec unity | 10m16s → **8m57s (-79.2 s / -12.9%)** | 1533 s → 579 s (2.6×) | `libExec.a` 2450 MB → 641 MB | | Exprs unity | 8m57s → **7m57s (-60.0 s / -11.2%)** | 2147 s → 1333 s | `libExprs.a` 2721 MB → 1392 MB | Jumbo-TU envelope: the largest Exec unity TU compiles in 32 s / 2.6 GB RSS standalone, the largest Exprs one in 25 s / 1.95 GB — both below the largest existing individual TU in the tree (3.9 GB), so `-jN` memory envelopes are unchanged. ### Risk and verification - **Unity changes TU grouping only.** The code commits riding along are hygiene: constants deduplicated with identical values, one constant scoped into its class, two renames, one `#pragma once`. No logic change. - **Archive symbol parity** (checked on the development branch): Exec keeps all external defined symbols — three weak linkonce_odr template instantiations dedup away, which is the point of unity, not a loss. Exprs likewise (one weak instantiation dedups; the `DictGetManyState` rename carries its `shared_ptr` machinery under the new name). - **This exact branch, rebased onto current master, full BE build from scratch** (macOS arm64, clang 20, `ENABLE_PCH=ON`, `ENABLE_UNITY_BUILD=ON`): **7981/7981 ninja edges, zero failures, `doris_be` links (325 MB).** Exec produces exactly 14 unity TUs and Exprs exactly 31, as advertised. This includes `pipeline/rec_cte_shared_state.cpp`, added upstream after the waves were measured — it lands inside an Exec unity batch via the existing `GLOB_RECURSE` with zero CMakeLists edits, which is the intended maintenance story. - **The first CI round of this PR did its job and caught two issues; both are fixed in the current revision.** 1. *BE UT lane, duplicate symbols at link*: three test files compile a src `.cpp` a second time by `#include`ing it (`function_variant_element_test`, `function_uuid_test`, `function_json_object_flatten_test`). Pre-unity this linked only by archive-member selectivity: the test object defines the symbols first and the library member is never pulled. A unity batch, however, is pulled in for its *siblings* and brings a second strong definition. The three `#include`d files (`function_variant_element.cpp`, `uuid.cpp`, `function_jsonb_transform.cpp`) are now `SKIP_UNITY_BUILD_INCLUSION` — individual archive members restore exactly the shadowing semantics master links with today. 2. *Performance lane (the one gcc lane), `-Werror=subobject-linkage`*: `function_variant_element_v2.cpp` held `OwnedPathSegment` in an anonymous namespace as a field type of the externally-visible `...Path::Impl`. gcc only raises `-Wsubobject-linkage` when the definition sits in an `#include`d file — which is what unity turns a `.cpp` into; clang has no such warning, so every local build was green. The struct moves to namespace scope (name unique to the TU); fixed at the source rather than SKIPped. - **The OFF path, checked on the same tree**: reconfiguring with `ENABLE_UNITY_BUILD=OFF` drops **all 51** `unity_*.cxx` entries from `compile_commands.json` (Exec 14, Exprs 31, the #66712 pilots 6) and the TU count goes 8464 → 9042 — the batches return to exactly their 629 member files. Reconfiguring back ON restores exactly the same 51 batches. The switch semantics themselves (including winning over a stale `CMAKE_UNITY_BUILD` cache) were established in #66712. - **These two targets have been building as unity TUs on the development branch since 2026-08-08**, through repeated full-tree builds and the BE UT builds that verified #66672 (the UT binaries link against these same target libraries). - The `file_scanner_v2_test.cpp` hunk was compile-verified standalone against this branch (`-fsyntax-only` with the test TU's full include closure). ### Proactive disclosure - **Cross-platform is the blind spot, closed by this PR's own CI** — every local build and measurement above is macOS arm64 + clang 20. With `ENABLE_UNITY_BUILD` defaulting ON since #66712, the Linux compile lanes and every regression pipeline in this PR's CI run against unity Exec/Exprs — that is the validation, and the first round proved it works: the BE UT and gcc lanes each caught one real unity interaction (detailed above), fixed in this revision. Escape hatches, in order: per-user `ENABLE_UNITY_BUILD=OFF`, per-file `SKIP_UNITY_BUILD_INCLUSION`, or a one-line default flip. - **The incremental-rebuild trade-off is real**: touching one `.cpp` inside a batch recompiles the whole batch (≤12 sources for Exec, ≤8 for Exprs; a batch compiles in ~25–32 s). This is why the heaviest, most-edited TUs (`operator.cpp`, `hashjoin_build_sink.cpp`, the aggregate families, `in.cpp`, `multiply.cpp`, …) are deliberately SKIPped and keep per-file granularity, and `ENABLE_UNITY_BUILD=OFF` restores it everywhere. - **The SKIP lists are coverage policy, not leftovers**: 7 Exec + 38 Exprs files stay individual on purpose — generated parsers (flex/bison/gperf), files whose file-scope macros would leak into siblings, and the heavy codegen TUs where merging saves no closure parse worth the jumbo-TU cost. A future file whose file-scope symbols clash inside a unity TU opts out the same one-line way. - Unity covers the *glue* of these targets, not the codegen-heavy families — the 30 heavy Exprs SKIPs mean the headline per-target ratios (2.6× Exec slot time) are earned on the batched part; the SKIPped monsters keep their cost and their per-file granularity.
6 participants
What problem does this PR solve?
Related PR: #66510, #66672
Problem Summary:
Most of the BE's cold-build time is not spent compiling our code — it is spent
re-parsing the same shared header closure once per small
.cpp. For the gluedirectories this ratio is extreme: the 51 InformationSchema scanners cost ~124 CPU s
(with PCH) of which almost everything is the closure re-parse; the ~58 http handlers
sum to ~5.6 min of slot time for a few thousand lines of handler logic.
CMake's built-in
UNITY_BUILD(CMake ≥ 3.16; we require 3.19.2) concatenates groupsof
.cppfiles into jumbo TUs, so a closure is parsed once per batch instead ofonce per file. This PR:
Adds the switch —
option(ENABLE_UNITY_BUILD ON)inbe/CMakeLists.txt,plumbed through
build.shandrun-be-ut.shexactly likeENABLE_PCH,overridable from the environment /
custom_env.sh. Every per-targetUNITY_BUILDproperty is set unconditionally from it -- including an explicitOFF, so the switch also wins over CMake's own
CMAKE_UNITY_BUILDin a reusedcache. Turn it OFF for per-file diagnostics, per-file tooling (clang-tidy,
coverage), or the finest-grained incremental rebuilds.
Pilots unity on three glue segments (and only there — everything else is
explicitly opted out per file, not by omission):
UNITY_BUILD_BATCH_SIZE 0).http/: ~58 handler TUs → 1 unity TU. The non-httpservice sources (service entry points, arrow_flight) are heterogeneous heavy TUs
that gain nothing from merging and stay individual.
index/: ~111 TUs sharing one CLucene-heavy closure →4 unity TUs of ≤ 32 sources (batch size bounds jumbo-TU size and memory).
Makes the merged TUs legal C++ with two hygiene commits that stand on their
own even without unity:
constants (
HEADER_JSON×16,TABLET_ID×7,SCHEMA_HASH×4, …). They move toone shared header as C++17 inline variables, values unchanged.
prefix_querydeclaredget_prefix_terms(IndexReader*)relying on aheader-level
CL_NS_USE(index)using-directive. Insidedoris::segment_v2thatunqualified name silently flips to
doris::segment_v2::IndexReaderas soon asany sibling source brings that type into scope — a landmine with or without
unity. The declaration now spells out
lucene::index::IndexReader.Fixes a latent bug unity exposed:
schema_scanner_helper.hopened with#ifndef _SCHEMA_SCANNER_HELPER_H_but never defined the macro (and one includesat outside the guard), so the guard never worked. Harmless while every TU
included it exactly once; breaks immediately under unity. Now
#pragma once.Keeps the compile-bench tooling honest under unity — with the default ON,
build.sh --compile-benchproduces a build directory whose sources are jumbo TUs,which two of the already-merged tools silently mis-read:
cut_impact.pydroppedevery unity dependency block (and with it all 211 pilot sources) from its TU set,
and
report.pyfiled each batch's time under the build directory instead of themodule it came from.
cut_impact.pynow refuses a unity build directory with therebuild instruction — its analysis needs per-source closures, and splitting a
batch's union back over its members would over-estimate instead of under-report —
report.pyattributes a unity TU to its target's directory, andrebuild_radius.pysays when its per-header counts are batch-granular.build-support/tests/test-compile-bench-unity.shpins all three behaviours.Files whose file-scope macros must not leak into unity siblings stay individual via
SKIP_UNITY_BUILD_INCLUSION:http_parser.cpp(CR/LF),be_thread_stack_action.cpp(
UNW_LOCAL_ONLY), and four index files (CL_MAX_PATHand friends,IS_CHINESE_CHAR,APPLY_FOR_PRIMITITYPE). A future file whose file-scope symbolsclash inside a unity TU can opt out the same way.
Measured results
All numbers from the development branch this series is split from (which also
carried the #66672 include cuts), macOS arm64 + clang 20,
-j14,ENABLE_PCH=ON, cold builds, back-to-back A/B:Per-segment slot time (sum of compile time the segment occupies across slots):
Why unity rather than more PCH: a serial cold micro-benchmark of the
InformationSchema target alone measured 9.9× with PCH (124 s → 12.5 s) and
14.6× without PCH (206 s → 14.1 s) — and unity without PCH still beats
individual TUs with PCH by 8.8×. Unity removes the repeated parse instead of
amortizing it, and the two compose.
Side effects on artifacts:
libInformationSchema.a334 MB → 29 MB,libStorage.a1492 MB → 1318 MB (linkonce_odr instantiations dedup inside each unity TU). The
largest unity TU peaks at 2.1 GB RSS — below the largest existing individual TU in
the tree (3.9 GB), so
-jNmemory envelopes are unchanged.Risk and verification
hygiene commits described above (constant dedup with identical values, one
qualified name, one include guard).
InformationSchema keeps all 2303 external defined symbols (plus 5 weak
unique_ptr<SchemaXxxScanner>instantiations that dedup), Service keeps all 4054,Storage keeps all external symbols with 8 weak linkonce_odr template
instantiations deduping away — which is the point of unity, not a loss.
(macOS arm64, clang 20,
ENABLE_PCH=ON,ENABLE_UNITY_BUILD=ON):8349/8349 ninja edges, zero failures,
doris_belinks. The six expected unityTUs (InformationSchema ×1, Service http ×1, storage/index ×4) all compile.
This includes
schema_tso_status_scanner.cpp, added upstream after the pilot wasmeasured — it lands inside the InformationSchema unity TU via the existing
GLOB_RECURSEwith zero CMakeLists edits, which is the intended maintenance story.ENABLE_UNITY_BUILD=OFFremoves everyunity_*.cxxentry fromcompile_commands.jsonand flips exactly 219 ninja edges — the three targets'per-file objects plus their archives and the final link, nothing else. All of them
compile per-file with zero failures and
doris_belinks again. The blast radiusof the switch is precisely the three pilot targets.
OFFalso wins over a native-unity cache: configuring with-DENABLE_UNITY_BUILD=OFF -DCMAKE_UNITY_BUILD=ONproduced 28 unity TUs across thethree pilot targets before the gate fix and 0 after, while the ordinary
-DENABLE_UNITY_BUILD=ONconfigure still produces exactly the 6 advertisedbatches (InformationSchema ×1, Service ×1, Storage ×4).
since 2026-08-08, through repeated full-tree builds and the BE UT builds that
verified [opt](build) Cut three more waves of hot include edges in the BE header graph #66672 (the UT binaries link against these same target libraries).
Proactive disclosure
PR's own CI closes it. Every local build and measurement above is macOS arm64 +
clang 20. Nothing here is platform-specific by construction, but with the default
ON, the Linux compile lanes and every regression pipeline in this PR's CI run
against unity builds — that is the validation. Please give the Linux gcc lane in
particular a look. If some environment trips over unity after merge, the
escape hatches are, in order: per-user
ENABLE_UNITY_BUILD=OFF(env orcustom_env.sh), per-fileSKIP_UNITY_BUILD_INCLUSION, or a one-line defaultflip — the infrastructure stays either way.
.cppinside a unitybatch recompiles the whole batch. For the glue chosen here a batch compiles in
~16–30 s, comparable to single mid-weight TUs elsewhere in the tree; and
ENABLE_UNITY_BUILD=OFFrestores per-file granularity for workflows that need it.This is also why the pilot targets are glue directories and not the hot-edit paths.
f2c-generated LAPACK sources and the snowball stemmers define clashing file-scope
statics (
static c__1and friends) — structurally un-unifiable without rewritinggenerated code — and contrib compiles once and rarely changes.
heavy heterogeneous TUs earns nothing (the closure parse is not the dominant cost
there) and risks monster TUs. Follow-up PRs extend unity to Exec, Exprs and the
remaining targets with the same SKIP discipline; on the development branch the
full rollout takes the same tree from 10m16s to 6m18s.
Release note
None
Check List (For Author)
Test
Behavior changed:
Does this need documentation?