morningman opened a new pull request, #66826:
URL: https://github.com/apache/doris/pull/66826
> Part of the BE cold-build / rebuild-radius reduction series tracked in
#66715 (6/6, final PR of the series).
### What
Take `storage/olap_common.h` off the every-TU dependency base: move its
three universally-consumed pieces (`int128_t`/`uint128_t` typedefs, `RowsetId`,
`FieldType`) into small dedicated headers, cut four side-door include edges
under the column/type base, and drop `storage/olap_common.h` from `pch.h`.
**The main win is incremental rebuild radius, not cold-build wall clock.**
Cold build is measured neutral (673.3s vs 674.1s A/B on the PCH removal); what
changes is how many TUs recompile when a storage-domain header is touched, and
how many preprocessed lines every function/expr TU pays.
### Why / mechanism
- `core/types.h -> binary_cast.hpp -> packed_int128.h -> olap_common.h`: the
deepest base header of the tree carried the whole storage domain because of two
`int128` typedefs. They move to `core/extended_types.h`; `RowsetId` moves to a
new `storage/rowset_id.h/.cpp` (method bodies out-of-line), `FieldType` to a
new `storage/field_type.h`. `olap_common.h` re-exports all three, so the 260
direct users see zero API change.
- `exprs/function/function.h` included three storage-domain headers (zonemap
condition, inverted index iterator, function parser). The three edges are
redundantly meshed: cutting any single one is worth almost nothing (-406 /
-15,455 / 0 preprocessed lines), cutting the group is -122,885 lines from every
function TU.
- Four side-door edges under the column/type base:
- `core/data_type/primitive_type.h` (and formerly `core/field.h`, see
drift note) included `util/json/path_in_data.h` -> `gen_cpp/segment_v2.pb.h`
(~11.6k lines) for one `using VariantMap = std::map<PathInData,
FieldWithDataType>` alias. A forward declaration suffices.
- `common/logging.h` included `util/uid_util.h` (-> `Types_types.h` +
`boost/uuid` -> `boost/tti`) so `TaggableLogger::tag` could name
`TUniqueId`/`PUniqueId` in an `if constexpr`. `std::is_same_v` works on
incomplete types.
- `util/pretty_printer.h` included `boost/algorithm/string.hpp` (~60k
lines) for one `boost::algorithm::join` and two `boost::enable_if_c`;
`runtime_profile.h` includes `pretty_printer.h`, so every TU with a profile
paid for it. Replaced by direct streaming and `std::enable_if_t`.
- `pch.h` drops `storage/olap_common.h` (it alone pulled 22 doris headers
into the PCH blast line).
### Measured effect (mother-branch pairing, clang20 / macOS arm64, `ninja -t
deps`-based radius)
| touch this header | dependent TUs before | after |
|---|---|---|
| `storage/olap_common.h` | **318 = every first-party TU** (via PCH) + a
219MB PCH rebuild | **182** |
| `util/uid_util.h` | **318 = every TU** | **196** |
| `util/json/path_in_data.h` | 238 | 172 |
| PCH blast line (doris headers in pch closure) | **31** | **9** |
`multiply.cpp` natural closure across the whole series: 432,112 -> 242,764
preprocessed lines (-43.8%); aws/S3 SDK, CLucene and `segment_v2.pb.h` are gone
from function-module closures entirely.
### Pre-existing defects fixed on the way
- `exprs/function/function_encryption.cpp`: statically out-of-bounds index
into `bool[4]` in the `arg_num==4` instantiation (indexed `[4]`); rewritten as
`if constexpr` dispatch.
- `storage/index/index_file_reader.h`: its CLucene warning suppression only
worked by include-order luck.
- `storage/segment/condition_cache.h`: uses `RowsetId` but never included a
header providing it (leaned on a neighbor's transitive include).
### Upstream drift absorbed during rebase
- #65561 (ColumnVariantV2) moved the `VariantMap` alias plus its
`util/json/path_in_data.h` include from `core/field.h` into the new
`core/value/variant/variant_field.h`, which `field.h` now includes — same heavy
edge, one hop longer. The cut is applied at the new location: `variant_field.h`
forward-declares `PathInData`, and the TUs that really instantiate the map
(`variant_field.cpp`, `variant_field_test.cpp`) include the real header
directly. One subtlety: `VariantField`'s class-body `= default` default
constructor was an inline definition, and with the key type forward-declared it
would instantiate the `VariantMap` destructor through the `unique_ptr` deleter
— it moves to the .cpp (declared `noexcept`, defaulted there); every other
special member was already out-of-line. `field.h` ends up with zero net change.
A whole-tree audit of "names `PathInData` without directly including its
header" (8 src + 19 test files) confirmed every one has an independent provider
(`column_var
iant.h` and the variant reader/writer headers include `path_in_data.h`
themselves); a second sweep for files that spell only `VariantMap`/`legacy_map`
caught one more — `column_variant_v2_test.cpp` value-constructs `VariantMap {}`
and now includes the header directly.
- `data_type_array_serde.cpp` grew a `FieldType::` use upstream (#66413
series) after the mother-branch closure sweep, compiling only through the PCH's
`olap_common.h`; with the PCH entry dropped it gets the direct
`storage/field_type.h` include (folded into the pch commit).
### Verification
- Full BE build in this PR's own tree (clang20 / macOS arm64, unity=ON,
PCH=ON, -j14): 7446/7446 targets, zero failures, `doris_be` links (319MiB).
- BE UT build + link (BUILD_TYPE_UT=Debug): 8503 targets green,
`doris_be_test` links (306MiB), zero duplicate/undefined symbols. The variant
suites touched by the drift absorption ran green: `VariantFieldTest.*` +
`ColumnVariantV2*` = 61/61 passed.
- Mother-branch verification of the same changes: closure-sweep 300/300 TUs
clean (each TU `-fsyntax-only` against its real include closure, no PCH symbol
leakage — tooling from #66616); 4 rounds of incremental rebuild; 3 rounds of BE
UT.
- The `SKIP_PRECOMPILE_HEADERS` / PCH interaction has a dedicated A/B:
PCH-drop is wall-clock neutral (673.3s vs 674.1s), so the radius win is free.
### Deliberately disclosed
- This PR's benefit shows up when *editing storage headers* and in per-TU
preprocessed size, not in cold-build totals — do not evaluate it by cold-build
wall clock.
- `olap_common.h` still re-exports the three moved headers; nothing was
migrated call-site-by-call-site. Peeling direct users off the re-export is
possible follow-up, not needed for the win.
- All measurements are clang20/macOS; Linux gcc/clang lines are covered by
this PR's CI. The three include cuts are pure edge removals verified by
closure-sweep, so platform risk concentrates in the two new headers
(`rowset_id.h`, `field_type.h`), which are plain moves.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
