Uh oh!
There was an error while loading. Please reload this page.
[opt](build) Decouple common/status.h from thrift/protobuf generated headers - #67036
Conversation
hello-stephen
commented
Aug 21, 2026
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
morningman
commented
Aug 21, 2026
run buildall |
hello-stephen
commented
Aug 21, 2026
TPC-H: Total hot run time: 17629 ms |
hello-stephen
commented
Aug 21, 2026
TPC-DS: Total hot run time: 84308 ms |
hello-stephen
commented
Aug 21, 2026
ClickBench: Total hot run time: 14.75 s |
hello-stephen
commented
Aug 21, 2026
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
hello-stephen
commented
Aug 21, 2026
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
…enerated headers Mechanical preparation wave, no include is removed yet (safe on master): - endian.h: define gbswap_16/32 with compiler builtins instead of leaning on the bswap_* macros that protobuf's stubs/port.h leaks into most TUs - hash_util.hpp: the std::hash specializations for TUniqueId/TNetworkAddress keep only declarations here (an incomplete type suffices); bodies move to uid_util.cpp / network_util.cpp where the thrift types are complete - IDataType::to_protobuf(PTypeDesc*) and the nine derived overrides move out of line; data_type.h forward-declares PTypeDesc/PTypeNode/PScalarType - add direct includes to every TU that named TStatusCode/TStatus/PStatus/ PValues or std::set/unordered_set but rode them in via common/status.h or hash_util.hpp Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0167LexPzAjQuu5yHvaH9hmo
…tobuf generated headers common/status.h is included by ~2450 of ~2500 BE TUs and carried gen_cpp/Status_types.h (thrift runtime) plus gen_cpp/types.pb.h (protobuf runtime) into every one of them: -44k preprocessed lines per TU wherever no other generated-header path exists. Surgery (zero runtime semantic change): - APPLY_FOR_THRIFT_ERROR_CODES gains the literal enum value per entry; status.cpp locks every value to the thrift enum with a static_assert, so Status.thrift stays the single source of truth (drift = compile error) - Status::create<stacktrace>(TStatus/PStatus) bodies move to status.cpp with 4 explicit instantiations (cold path: RPC error conversion) - code_as_string() moves to status.cpp (cold path: error printing) - both generated includes are dropped; TStatus joins PStatus as a forward declaration; exception.h drops its unused Status_types.h include Guards: check-header-deps.py forbids status.h/exception.h from reaching gen_cpp/* (empty whitelist), the exec_env whitelist empties out, the thread_context whitelist loses Status_types.h, and REVERSE_REACH_BASELINES locks Status_types.h at its new reach (37 TUs, down from 2453). syntax_sweep.py gains --include-tests: be/test builds without a PCH, so the UT line always compiles natural closures and must be sweepable. Probes (clang -E, macOS arm): status.h closure 147,838 -> 103,883 lines (-29.7%); util/bitmap.cpp -43,969; text-graph reach: Status_types.h 2453 -> 37 TUs, types.pb.h 2458 -> 1791. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0167LexPzAjQuu5yHvaH9hmo
hash_util.hpp rides into most of the backend through string_ref.h, column_string.h, vdatetime_value.h and storage/olap_common.h. After the std::hash specializations for TUniqueId/TNetworkAddress moved their bodies out of line (previous commit), the Types_types.h include carries nothing this header uses: dropping it removes the whole thrift runtime from every TU whose only path to it was hash_util (closure 178,644 -> 131,801 preprocessed lines, -26%). check-header-deps.py forbids hash_util.hpp from reaching gen_cpp/* so the edge cannot silently regrow. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0167LexPzAjQuu5yHvaH9hmo
morningman
commented
Sep 1, 2026
run buildall |
hello-stephen
commented
Sep 1, 2026
TPC-H: Total hot run time: 16922 ms |
hello-stephen
commented
Sep 1, 2026
TPC-DS: Total hot run time: 81648 ms |
hello-stephen
commented
Sep 1, 2026
ClickBench: Total hot run time: 14.55 s |
hello-stephen
commented
Sep 2, 2026
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
hello-stephen
commented
Sep 2, 2026
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Uh oh!
There was an error while loading. Please reload this page.
PR approved by anyone and no changes requested. |
PR approved by at least one committer and no changes requested. |
Upstream apache#67036 decoupled common/status.h from thrift generated headers. Branch tests use TStatusCode via the old ride-along include; add the direct include.
What problem does this PR solve?
Issue Number: none
Related PR: #66901 (header-hygiene gate this PR extends), #66715 (BE build-speed umbrella, closed)
Problem Summary:
common/status.his included by ~2450 of ~2500 BE TUs and carriedgen_cpp/Status_types.h(the whole thrift runtime) plusgen_cpp/types.pb.h(the protobuf runtime) into every one of them. For any TU whose only path to
the generated headers was status.h, that is ~44k preprocessed lines of pure
tax per TU — paid in every build without a usable PCH: the Linux BE UT line
(be/test builds with no PCH),
ENABLE_PCH=OFF(gcc) builds, clangd/clang-tidy,and every incremental build after touching
Status.thrift/types.proto.This PR decouples status.h/exception.h (and
util/hash_util.hpp) from thegenerated headers, with zero runtime semantic change:
APPLY_FOR_THRIFT_ERROR_CODESnow carries the literal enumvalue per entry;
status.cpplocks every value to the thrift enum with astatic_assert, soStatus.thriftstays the single source of truth — anydrift is a compile error.
Status::create<stacktrace>(TStatus/PStatus)move out of line with 4 explicit instantiations (cold path: RPC error
conversion),
code_as_string()moves out of line (cold path: errorprinting), and both generated includes are dropped (
TStatusjoinsPStatusas a forward declaration).Status_types.hinclude.std::hash<TUniqueId/TNetworkAddress/pair>specializations keep only their declarations here (this header stays the
earliest carrier, so they remain visible before any implicit
instantiation); the bodies move to
uid_util.cpp/network_util.cppwherethe types are complete. The
Types_types.hinclude is then dropped.endian.hgets its owngbswap_16/32builtins instead of leaning on thebswap_*macros thatprotobuf's
stubs/port.hhappened to leak into most TUs;IDataType::to_protobuf(PTypeDesc*)and the nine derived overrides moveout of line (data_type.h forward-declares the pb types); every TU that
named
TStatusCode/TStatus/PStatus/PValuesorstd::set/std::unordered_setvia ride-along gets its direct include.Numbers (clang -E, macOS arm; text include-graph reach over be/src+be/test):
common/status.hclosureutil/hash_util.hppclosureutil/bitmap.cppnatural closure (representative winner)gen_cpp/Status_types.hreachgen_cpp/types.pb.hreachCold build wall-clock with clang +
ENABLE_PCH=ONis neutral (687.3s ->693.6s, +0.9%, paired back-to-back
--compile-bench -j 6runs, withinrun-to-run noise) — expected, since pch.h itself still includes the generated
headers. The wins are the no-PCH worlds and the incremental radius: editing
Status.thriftno longer invalidates essentially every TU there.Guards (extends #66901):
check-header-deps.pynow forbidsstatus.h/exception.h/hash_util.hpp from reaching
gen_cpp/*(emptywhitelists), the exec_env gen_cpp whitelist empties out, thread_context loses
its
Status_types.hentry, andREVERSE_REACH_BASELINESpins the threegenerated headers at their new reach so the tax cannot silently regrow.
syntax_sweep.pygains--include-testsso the UT line's natural closuresare sweepable.
Release note
None
Check List (For Author)
Test
-fsyntax-onlynatural-closure sweeps (PCH stripped) at threescopes, all zero regressions vs pre-surgery baselines: Release
unity scope (315 TUs), UT test scope with
--include-tests(1167 be/test TUs), and a per-file sweep of all 1497 be/src
sources (defeats unity masking);
build.sh --becompile +doris_belink green;status_test.cpp/exception_test.cppcompiled to .o with theUT flag set;
#include <gen_cpp/types.pb.h>into status.h makes
check-header-deps.pyfail as intended;Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)
🤖 Generated with Claude Code
https://claude.ai/code/session_0167LexPzAjQuu5yHvaH9hmo