Skip to content

Commit b9ee837

Browse files
morningmanclaude
andauthored
[opt](build) Enable unity builds for the remaining BE targets (#66789)
> 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; #66776 extended it to Exec and Exprs. > This PR finishes the line: unity builds for every remaining BE target where it > helps, plus a fail-loud guard for the skip lists. It is the last PR of the unity > sub-series (the remaining two PRs of the batch are orthogonal: extern-template > convergence and olap_common decoupling). ### What problem does this PR solve? Related PR: #66510, #66712, #66776 Problem Summary: Same mechanism as #66712/#66776: 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. This PR applies it to the twelve remaining targets where the win is real, and writes down (in-tree) the two places where it is not. The eleven commits, in fix-then-enable pairs: 1. **Deduplicate cloud/storage file-scope names that clash under unity** — identical copies hoisted to one shared definition (warm-up metric window spans -> `bvar_windowed_adder.h`; the "D0R1" segment footer magic -> `segment/common.h`; the `path_allocated_bytes` PathInData accounting helper that #66204 landed in two variant v2 TUs -> `util/json/path_in_data.h`), a divergent copy renamed instead (`create_ext_meta_io_context` keeps file-cache stats where its siblings reset them); **plus one real latent bug**: `merger.cpp` threw with `INTERNAL_ERROR` bound to `PPlanFragmentCancelReason::INTERNAL_ERROR` (=3) from `types.pb.h` instead of `ErrorCode::INTERNAL_ERROR` (=6) — no `ErrorCode` using-directive was in scope. Qualified; those compaction-failure statuses now carry the intended code. 2. **Unity for Cloud (batch 12) and Storage target-wide (batch 12)** — Storage's previous index/-only inverse-selection flips to graded opt-outs. Cloud: 37 members -> 4 batches. Storage: 232 members -> 20 batches. 3. **Deduplicate format_v2 file-scope helpers** that clash under unity. 4. **Unity for the format_v2 half of Format (batch 8)** — the v1 `format/` tree is slated for removal, so every v1 file stays individual rather than paying the v1/v2 twin-symbol dedup a joint unity would need. 5. **Resolve runtime/load/io/util file-scope clashes** — including two more latent bugs found by merged TUs: `column_predicate.h` had the same unqualified `INTERNAL_ERROR` mis-binding as `merger.cpp`, and `vdatetime_value.h` defined a `static RE2` in the header (one RE2 construction per including TU at startup; now a C++17 inline variable, one program-wide). `runtime_profile.h` also carried two never-referenced private fields; removed. 6. **Unity for Runtime, Load, IO and Util** (batch 12/12/16/24 by member weight). 7. **Add missing include guards to two more headers** (`cgroup_cpu_ctl.h`, `complex_type_deserialize_util.h`) — found the same way as the two in #66776. 8. **Unity for Core, Agent, Udf, Common, ann_index** — and the Service negative result recorded in-tree (see below). 9. **Unity for the thrift half of DorisGen (batch 8)** — protobuf `.cc` files stay individual structurally: protoc emits identical file-scope statics (`schemas[]`, `file_default_instances[]`) in every one. 10. **Evict `file_cache_lru_tool.cpp` from `libIO.a`** — a standalone tool with its own `main()` must not be an archive member once unity can merge it into an object the `doris_be` link actually pulls in. 11. **Fail the configure when a unity skip entry goes stale** — `set_source_files_properties()` silently ignores nonexistent paths, so a rename would quietly dissolve a skip entry and the file would rejoin its batch with no diagnostic. All thirteen skip lists (including the merged Exec/Exprs and pilot ones) now go through `doris_skip_unity_inclusion()`, which FATAL_ERRORs on a nonexistent entry (generated `GENSRC_DIR` paths exempt — they legitimately do not exist on a fresh configure). ### Benefit Measured on the development branch this series is split from (arm64 macOS, clang 20, `-j14`, PCH on, cold builds, same-machine paired runs): - this wave (on top of the #66776 state): **7m57s -> 6m18s (-99s / -20.7%)**, sum-of-TU CPU (user) 98.7 -> 76.7 min, compile units 7810 -> 7280. - static archives collapse as duplicate DWARF / weak-template copies merge (measured on this branch, unity OFF vs ON): libStorage.a 1592 -> 514 MB, libCloud.a 441 -> 107 MB, libRuntime.a 356 -> 122 MB, libLoad.a 311 -> 100 MB, libFormat.a 1057 -> 760 MB, libCore.a 371 -> 182 MB. Less disk thrash per build, faster archiving and linking. - the four latent bugs above found and fixed (two of them wrong-code). ### Verification on this branch (arm64 macOS, clang 20, ENABLE_PCH=ON) - unity=ON full build: zero failures across all 7927 TUs, `doris_be` links (319 MB). One upstream-drift clash surfaced and fixed first (the #66204 `path_allocated_bytes` twins — folded into the dedup commit). - unity coverage after this PR: 110 batches / 1224 member files tree-wide, of which this PR adds 63 batches / 696 members (per-target counts in the commit messages); `compile_commands.json` agrees. - `ENABLE_UNITY_BUILD=OFF` reconfigure: unity entries drop to zero and the TU count goes 7927 -> 9041 = exactly the 110 batches dissolving into their 1224 members; full rebuild green, `doris_be` relinks -> the switch's blast radius is precisely the unity config. - BE UT (`BUILD_TYPE_UT=Debug`): doris_be_test compiles and links clean — this specifically exercises the four "a test #includes the src .cpp" opt-outs below. - fail-loud helper: deliberately renaming one skip entry makes the configure fail with `unity skip entry does not exist (renamed or moved?): <path>` and the offending CMakeLists line; reverting restores a clean configure. ### First CI round: two reds, both root-caused and fixed in-tree - **ASAN Compile**: libstdc++-15 annotates loops in its own headers with `#pragma GCC unroll`; under `-O1` + sanitizers + coverage the unroller punts and clang's `-Wpass-failed` (a missed-*hint* diagnostic with no correctness signal) became fatal under `-Werror` once a storage unity batch changed the emission context. Fixed by `-Wno-pass-failed` for clang tree-wide. - **BE UT**: `FSFileCacheLeakCleanerTest.remove_orphan_and_tmp_files` — a pre-existing test-helper race: it waited for the meta-store *write queue* to drain, but the async worker dequeues before it issues the rocksdb Put, so the leak scan could still count zero metadata blocks and skip cleanup. The unity-batched IO objects shifted thread timing on the ASAN runner and hit the window (this test's only failure in its last 40 CI runs is this PR's round — the race is upstream, the timing shift is ours). Fixed by polling the store itself instead of the queue size. ### Deliberate opt-outs (not omissions) - **Per-file opt-outs** (~50 files across the targets, each annotated in its CMakeLists): file-scope macro leakers, the heaviest template-instantiation TUs (so no batch inherits their weight), `ENABLE_VARIANT_NESTED_GROUP` / `ENABLE_TDE` module swaps (unaudited out-of-tree sources), and generated protobuf `.cc`. - **Four files a test compiles a second time by `#include`-ing the .cpp** (`storage/compaction/collection_statistics.cpp`, `runtime/descriptors.cpp`, `core/column/column_variant.cpp`, `core/data_type/convert_field_to_type.cpp`): the test object must shadow a never-pulled archive member, but a unity batch is pulled in for its siblings and the linker sees a duplicate definition. Same failure class the first CI round of #66776 hit; this time the whole tree was swept up front (`grep -rn '#include ".*\.cpp"' be/test`) and the remaining hits are either already opted out, structurally outside unity scope, or test-side files. - **Service stays at http-only unity**: widening to the whole target at batch 8 was tried and measured **slower** (service segment slot time 104s -> 132s) — splitting the single http jumbo repays the shared-closure parse per batch and the heterogeneous non-http TUs gain too little to cover it. The CMakeLists comment records this so nobody retries it blind. - **contrib (openblas/clucene) deliberately untouched**: openblas is f2c output — 2055 files with colliding `static c__1` file-scope constants, structurally un-unifiable. ### Cross-platform note (please watch the gcc pipeline) All local verification is macOS/clang. gcc-only hazards were audited up front: the `-Wsubobject-linkage` pattern that bit #66776 (an anonymous-namespace type as a member of an external-linkage class turns into an error only once unity makes the file `#include`-d) was swept across all 627 files entering unity scope — every anonymous-namespace type usage found is function-local, so no hazard — but the sweep is heuristic, so the first Performance (gcc) CI round is worth watching. ### Release note None ### Check List (For Author) - Test - [x] Manual test (add detailed scripts or steps below) - Full BE build with unity ON: all 7927 TUs green, `doris_be` links. - `ENABLE_UNITY_BUILD=OFF` reconfigure + full rebuild green; TU accounting closes exactly (7927 -> 9041); flip back ON restores. - BE UT (`BUILD_TYPE_UT=Debug`): doris_be_test compiles and links with zero duplicate symbols. - Deliberately breaking a unity skip entry fails the configure with the new fail-loud diagnostic; reverting restores a clean configure. - Behavior changed: - [x] Yes. Two latent wrong-code fixes surfaced by unity merges (details in the commit messages): `merger.cpp` and `column_predicate.h` threw with `INTERNAL_ERROR` accidentally bound to `PPlanFragmentCancelReason` (=3); they now carry `ErrorCode::INTERNAL_ERROR` as intended. Also `vdatetime_value.h`'s header-defined `static RE2` became a C++17 inline variable: one instance program-wide instead of one per including TU (same matching behavior, less startup work and memory). - Does this need documentation? - [x] No. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01Gdfkk7RqgD5e3Uv7bTM3NV --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
1 parent 75ddba6 commit b9ee837

50 files changed

Lines changed: 336 additions & 128 deletions

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

be/CMakeLists.txt

Lines changed: 26 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -409,7 +409,14 @@ if (COMPILER_CLANG)
409409
-Wthread-safety)
410410
add_compile_options(-Wno-gnu-statement-expression
411411
-Wno-implicit-float-conversion
412-
-Wno-sign-conversion)
412+
-Wno-sign-conversion
413+
# libstdc++-15 puts `#pragma GCC unroll` on loops in its own
414+
# headers (bits/stl_algobase.h); when a sanitizer/coverage
415+
# build compiles at low optimization the unroller punts and
416+
# clang's -Wpass-failed turns that missed *hint* into an
417+
# -Werror failure. The diagnostic carries no correctness
418+
# signal, so drop it.
419+
-Wno-pass-failed)
413420
if (USE_LIBCPP)
414421
add_compile_options($<$<COMPILE_LANGUAGE:CXX>:-stdlib=libc++>)
415422
add_definitions(-DUSE_LIBCPP)
@@ -981,6 +988,24 @@ function(pch_reuse target)
981988
endif()
982989
endfunction(pch_reuse target)
983990

991+
# Every unity opt-out goes through this helper. set_source_files_properties()
992+
# silently ignores paths that do not exist, so a skip entry going stale after
993+
# a rename or move would quietly re-join its unity batch -- slower builds, or
994+
# new file-scope clashes, with no diagnostic anywhere. Fail the configure
995+
# instead. Entries under GENSRC_DIR are exempt: they are emitted by
996+
# add_custom_command at build time (wkt_lex.l.cpp, the generated
997+
# thrift/protobuf sources) and legitimately do not exist on a fresh configure.
998+
function(doris_skip_unity_inclusion)
999+
foreach(entry IN LISTS ARGN)
1000+
string(FIND "${entry}" "${GENSRC_DIR}" gensrc_prefix_pos)
1001+
if(NOT gensrc_prefix_pos EQUAL 0 AND NOT EXISTS "${entry}")
1002+
message(FATAL_ERROR
1003+
"unity skip entry does not exist (renamed or moved?): ${entry}")
1004+
endif()
1005+
endforeach()
1006+
set_source_files_properties(${ARGN} PROPERTIES SKIP_UNITY_BUILD_INCLUSION ON)
1007+
endfunction()
1008+
9841009

9851010
add_subdirectory(${SRC_DIR}/agent)
9861011
add_subdirectory(${SRC_DIR}/common)

be/src/agent/CMakeLists.txt

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -29,4 +29,9 @@ endif()
2929

3030
add_library(Agent STATIC ${AGENT_SOURCES})
3131

32-
pch_reuse(Agent)
32+
pch_reuse(Agent)
33+
34+
# Unity build: nine homogeneous task-worker glue TUs sharing the agent header
35+
# closure; batch 0 merges them into a single unity TU.
36+
set_target_properties(Agent PROPERTIES UNITY_BUILD ${DORIS_UNITY_BUILD}
37+
UNITY_BUILD_BATCH_SIZE 0)

be/src/agent/cgroup_cpu_ctl.h

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,8 @@
1515
// specific language governing permissions and limitations
1616
// under the License.
1717

18+
#pragma once
19+
1820
#include <fcntl.h>
1921
#include <sys/stat.h>
2022
#include <sys/types.h>

be/src/cloud/CMakeLists.txt

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,3 +22,15 @@ file(GLOB_RECURSE SRC_FILES CONFIGURE_DEPENDS *.cpp)
2222
add_library(Cloud STATIC ${SRC_FILES})
2323

2424
pch_reuse(Cloud)
25+
26+
# Unity build: homogeneous cloud glue TUs sharing the CloudStorageEngine /
27+
# meta-service RPC header closure. Batch 12 bounds jumbo-TU size and memory.
28+
# The two http action TUs stay individual: sibling TUs' file-scope
29+
# `using namespace ErrorCode` makes the unscoped HttpStatus enum constants
30+
# (OK, NOT_FOUND, ...) shadow ErrorCode variables under -Wshadow -Werror.
31+
set(CLOUD_UNITY_SKIP
32+
${CMAKE_CURRENT_SOURCE_DIR}/cloud_compaction_action.cpp
33+
${CMAKE_CURRENT_SOURCE_DIR}/injection_point_action.cpp)
34+
doris_skip_unity_inclusion(${CLOUD_UNITY_SKIP})
35+
set_target_properties(Cloud PROPERTIES UNITY_BUILD ${DORIS_UNITY_BUILD}
36+
UNITY_BUILD_BATCH_SIZE 12)

be/src/cloud/cloud_internal_service.cpp

Lines changed: 1 addition & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1002,12 +1002,7 @@ bvar::Adder<uint64_t> g_file_cache_warm_up_rowset_wait_for_compaction_num(
10021002
bvar::Adder<uint64_t> g_file_cache_warm_up_rowset_wait_for_compaction_timeout_num(
10031003
"file_cache_warm_up_rowset_wait_for_compaction_timeout_num");
10041004

1005-
// Per-job windowed metrics for target BE
1006-
// bvar::Window enforces MAX_SECONDS_LIMIT = 3600, so the longest window is 1h.
1007-
static constexpr int WINDOW_5M = 300;
1008-
static constexpr int WINDOW_30M = 1800;
1009-
static constexpr int WINDOW_1H = 3600;
1010-
1005+
// Per-job windowed metrics for target BE (window spans shared via bvar_windowed_adder.h)
10111006
MBvarWindowedAdder g_warmup_ed_finish_segment_num("warmup_ed_finish_segment_num", {"job_id"},
10121007
{WINDOW_5M, WINDOW_30M, WINDOW_1H}, false);
10131008
MBvarWindowedAdder g_warmup_ed_finish_segment_size("warmup_ed_finish_segment_size", {"job_id"},

be/src/cloud/cloud_warm_up_manager.cpp

Lines changed: 1 addition & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -118,12 +118,7 @@ bvar::Adder<int64_t> g_file_cache_warm_up_job_num("file_cache_warm_up_job_num");
118118
bvar::LatencyRecorder g_file_cache_warm_up_rowset_wait_for_compaction_latency(
119119
"file_cache_warm_up_rowset_wait_for_compaction_latency");
120120

121-
// Per-job windowed metrics for source BE
122-
// bvar::Window enforces MAX_SECONDS_LIMIT = 3600, so the longest window is 1h.
123-
static constexpr int WINDOW_5M = 300;
124-
static constexpr int WINDOW_30M = 1800;
125-
static constexpr int WINDOW_1H = 3600;
126-
121+
// Per-job windowed metrics for source BE (window spans shared via bvar_windowed_adder.h)
127122
MBvarWindowedAdder g_warmup_ed_requested_segment_num("warmup_ed_requested_segment_num", {"job_id"},
128123
{WINDOW_5M, WINDOW_30M, WINDOW_1H}, false);
129124
MBvarWindowedAdder g_warmup_ed_requested_segment_size("warmup_ed_requested_segment_size",

be/src/common/CMakeLists.txt

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,22 @@ add_library(Common STATIC ${SRC_FILES})
2323

2424
pch_reuse(Common)
2525

26+
# Unity build for the small common TUs. Seven files leak file-scope macros
27+
# (SET_FIELD/UPDATE_FIELD, MEMORY_SANITIZER, DW_* tables, the DEFINE_*_METRIC
28+
# families, USE_PHDR_CACHE/UNW_LOCAL_ONLY) and stay individual -- that set
29+
# also keeps the three same-name metric-hook statics (_s_hook_name) apart.
30+
set(COMMON_UNITY_SKIP
31+
${CMAKE_CURRENT_SOURCE_DIR}/config.cpp
32+
${CMAKE_CURRENT_SOURCE_DIR}/demangle.cpp
33+
${CMAKE_CURRENT_SOURCE_DIR}/dwarf.cpp
34+
${CMAKE_CURRENT_SOURCE_DIR}/metrics/doris_metrics.cpp
35+
${CMAKE_CURRENT_SOURCE_DIR}/metrics/jvm_metrics.cpp
36+
${CMAKE_CURRENT_SOURCE_DIR}/metrics/system_metrics.cpp
37+
${CMAKE_CURRENT_SOURCE_DIR}/phdr_cache.cpp)
38+
doris_skip_unity_inclusion(${COMMON_UNITY_SKIP})
39+
set_target_properties(Common PROPERTIES UNITY_BUILD ${DORIS_UNITY_BUILD}
40+
UNITY_BUILD_BATCH_SIZE 0)
41+
2642
# Generate env_config.h according to env_config.h.in
2743
configure_file(${CMAKE_CURRENT_SOURCE_DIR}/env_config.h.in ${GENSRC_DIR}/common/env_config.h)
2844
target_include_directories(Common PUBLIC ${GENSRC_DIR}/common/)

be/src/core/CMakeLists.txt

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -32,3 +32,41 @@ pch_reuse(Core)
3232
# instantiations below without definitions. Opt this single TU out of the PCH.
3333
set_source_files_properties(${CMAKE_CURRENT_SOURCE_DIR}/wide_integer_from_double.cpp
3434
PROPERTIES SKIP_PRECOMPILE_HEADERS ON)
35+
36+
# Unity build: column/data_type/serde glue TUs share the core header closure.
37+
# Opt-outs:
38+
# - column/column_vector.cpp: the heaviest template-instantiation TU in the
39+
# target (~40s alone); it would dominate any batch it joins
40+
# - three macro-leak files (WRITE_INTEGRAL_COLUMN_TO_ORC,
41+
# DECLARE_DECIMAL_COMPARISON, DELARE_DATE_ADD_INTERVAL)
42+
# - wide_integer_from_double.cpp: macro-leak, and it is the PCH-skipped
43+
# explicit-instantiation TU above -- it must keep its own compile
44+
# - data_type_serde.cpp: holds member-level explicit instantiations of serde
45+
# members whose class-level explicit instantiations live in the per-type
46+
# serde TUs; merging both forms into one TU is a duplicate explicit
47+
# instantiation
48+
# - the value/variant/ family (9s of slot time in total): copied file-local
49+
# helpers with diverging signatures (require_bytes, write_unsigned, ...) --
50+
# not worth renaming for the gain
51+
# - two files that tests compile a second time by #including the .cpp
52+
# (column/column_variant.cpp, data_type/convert_field_to_type.cpp): the
53+
# test object must shadow a never-pulled archive member, but a unity batch
54+
# is pulled in for its siblings and the linker sees a duplicate definition
55+
set(CORE_UNITY_SKIP
56+
${CMAKE_CURRENT_SOURCE_DIR}/column/column_vector.cpp
57+
${CMAKE_CURRENT_SOURCE_DIR}/column/column_variant.cpp
58+
${CMAKE_CURRENT_SOURCE_DIR}/data_type/convert_field_to_type.cpp
59+
${CMAKE_CURRENT_SOURCE_DIR}/data_type_serde/data_type_number_serde.cpp
60+
${CMAKE_CURRENT_SOURCE_DIR}/data_type_serde/data_type_serde.cpp
61+
${CMAKE_CURRENT_SOURCE_DIR}/field.cpp
62+
${CMAKE_CURRENT_SOURCE_DIR}/value/vdatetime_value.cpp
63+
${CMAKE_CURRENT_SOURCE_DIR}/value/variant/variant_batch_builder.cpp
64+
${CMAKE_CURRENT_SOURCE_DIR}/value/variant/variant_canonical.cpp
65+
${CMAKE_CURRENT_SOURCE_DIR}/value/variant/variant_field.cpp
66+
${CMAKE_CURRENT_SOURCE_DIR}/value/variant/variant_metadata.cpp
67+
${CMAKE_CURRENT_SOURCE_DIR}/value/variant/variant_scalar.cpp
68+
${CMAKE_CURRENT_SOURCE_DIR}/value/variant/variant_value.cpp
69+
${CMAKE_CURRENT_SOURCE_DIR}/wide_integer_from_double.cpp)
70+
doris_skip_unity_inclusion(${CORE_UNITY_SKIP})
71+
set_target_properties(Core PROPERTIES UNITY_BUILD ${DORIS_UNITY_BUILD}
72+
UNITY_BUILD_BATCH_SIZE 12)

be/src/core/data_type_serde/complex_type_deserialize_util.h

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,8 @@
1515
// specific language governing permissions and limitations
1616
// under the License.
1717

18+
#pragma once
19+
1820
#include "core/data_type_serde/data_type_serde.h"
1921
#include "core/string_ref.h"
2022

be/src/core/value/vdatetime_value.h

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -219,7 +219,9 @@ static constexpr uint32_t DATEV2_YEAR_WIDTH = 23;
219219
static constexpr uint32_t DATETIMEV2_YEAR_WIDTH = 18;
220220
static constexpr uint32_t DATETIMEV2_MONTH_WIDTH = 4;
221221

222-
static RE2 time_zone_offset_format_reg(R"(^[+-]{1}\d{2}\:\d{2}$)");
222+
// One shared instance program-wide (was `static`, i.e. one copy constructed
223+
// per including TU). Visiting is thread-safe.
224+
inline RE2 time_zone_offset_format_reg(R"(^[+-]{1}\d{2}\:\d{2}$)");
223225

224226
uint8_t mysql_week_mode(uint32_t mode);
225227

0 commit comments

Comments
 (0)