[improvement](be) Optimize bit unpacking with PDEP#65738
Open
HappenLee wants to merge 6 commits into
Open
Conversation
### What problem does this PR solve?
Issue Number: None
Related PR: None
Problem Summary: BitPacking decoded every full 32-value batch with scalar shifts and masks even on x86 CPUs that support BMI2 and AVX2. Add a PDEP-based unpacker for uint8_t, uint16_t, and uint32_t batches, use AVX2 widening for narrow values, and retain the existing scalar path for unsupported CPUs, architectures, types, and remainder values.
### Release note
Improve bit-packed integer decoding performance on BMI2 and AVX2 capable x86 CPUs.
### Check List (For Author)
- Test: Unit Test
- ./run-be-ut.sh -j 48 --run --filter=BitPackingTest.*
- Behavior changed: Yes, full 32-value bit-unpack batches use PDEP and AVX2 when supported; decoded results and fallback behavior are unchanged.
- Does this need documentation: No
### What problem does this PR solve?
Issue Number: None
Related PR: None
Problem Summary: Add a benchmark that compares scalar 32-value bit unpacking with the production PDEP and AVX2 implementation for uint8_t, uint16_t, and uint32_t across every supported bit width. Validate scalar and PDEP outputs before measuring either implementation.
### Release note
None
### Check List (For Author)
- Test: Manual test
- ./build.sh --benchmark -j 48
- benchmark_test --benchmark_filter=BM_.*Unpack --benchmark_min_time=0.01s
- build-support/run-clang-tidy.sh --build-dir be/build_RELEASE --files be/benchmark/benchmark_main.cpp
- Behavior changed: No
- Does this need documentation: No
### What problem does this PR solve?
Issue Number: None
Related PR: None
Problem Summary: Parameterize the PDEP unpack benchmark with 4K, 256K, and 1M values, compare the scalar kernel, direct PDEP kernel, and the actual BitPacking entry point, and validate optimized output against the scalar reference before measuring.
### Release note
None
### Check List (For Author)
- Test: Manual test
- Built benchmark_test and ran the uint32_t 256K/1M scalar, PDEP, and actual-path benchmark cases
- Behavior changed: No
- Does this need documentation: No
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
### What problem does this PR solve? Issue Number: None Related PR: apache#65738 Problem Summary: The generic PDEP unpack implementation remains functionally valid for 16-32 bit values, but repeated Release benchmarks at 4K, 256K, and 1M values showed non-monotonic performance and repeatable regressions at several high widths. Keep the generic implementation available to the benchmark, while limiting the production dispatch to BIT_WIDTH < 16. Wider values conservatively retain the scalar implementation instead of using a CPU- and working-set-specific width allowlist. ### Release note Use the PDEP bit-unpack optimization only for bit widths below 16. ### Check List (For Author) - Test: Unit Test / Manual test - ./run-be-ut.sh -j 48 --run --filter=BitPackingTest.* - Release benchmark_test for uint32_t widths 16-32 at 4K, 256K, and 1M values - build-support/check-format.sh - build-support/run-clang-tidy.sh --build-dir be/build_Release --files be/benchmark/benchmark_main.cpp be/test/util/bit_packing_test.cpp - Behavior changed: Yes, bit widths 16 and above retain the scalar unpack path - Does this need documentation: No
Contributor
Author
|
/review |
Contributor
Author
|
run buildall |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Automated review result: request changes for one blocking performance regression.
Critical checkpoint conclusions:
- Goal and correctness: The patch is focused and the PDEP layouts, exact batch bounds, returned cursor, truncated input, scalar remainder, and unsupported-CPU fallback all match the existing decoding contract in code review and independent all-width models. However, the optimization goal is not met across Doris's supported x86 population because the feature-only dispatch in MAIN-01 enables a severe regression on pre-Zen3 AMD CPUs.
- Scope and parallel paths: The five changed files are cohesive. The production chain through BatchedBitReader, Parquet booleans, and RleBatchDecoder was traced; the separate dictionary-decoding API is unchanged and does not require a semantic parallel update.
- Concurrency, lifecycle, memory, and errors: The kernel is stateless, owns no memory, adds no threads/locks/static-lifetime dependencies, and preserves decoder short-read failure behavior. No concurrency, lifecycle, MemTracker, or Status issue was found.
- Configuration, compatibility, persistence, and transactions: No configuration, persisted/storage format, FE-BE protocol, transaction, or rolling-upgrade boundary changes are involved; non-x86 builds, compilers other than GCC/Clang, and CPUs without BMI2+AVX2 retain the scalar implementation.
- Performance: MAIN-01 is blocking. A CPU-family profitability gate and representative AMD benchmarking are required before capability-only PDEP dispatch is safe.
- Tests and results: The new BE unit test is registered and covers all widths, two full batches plus a remainder, and a truncated buffer. It does not force the optimized ISA path on a feature-lacking runner, and the direct benchmark is opt-in and Intel-only; add capability-gated direct coverage and representative AMD performance coverage. Per the review-run contract I did not build or run tests locally; formatter checks were green, while the observed macOS BE-UT failure occurred before compilation because that runner selected JDK 25 instead of JDK 17.
- Observability and user focus: No additional logging or metrics are needed for this local stateless kernel. No additional user-provided review focus was supplied.
Review completion: complete after two-agent full coverage, a separate risk-focused pass, deduplication, and convergence. One inline issue is submitted; no other unresolved suspicious point remains.
Contributor
TPC-H: Total hot run time: 29574 ms |
Contributor
TPC-DS: Total hot run time: 177350 ms |
Contributor
ClickBench: Total hot run time: 25.24 s |
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
### What problem does this PR solve? Issue Number: None Related PR: apache#65738 Problem Summary: PDEP is supported by AMD Zen+ and Zen 2, but those CPUs execute it as a slow microcoded instruction. Add a BE configuration switch so deployments on those CPUs can retain the scalar bit-unpacking path, while keeping BMI2 optimized paths enabled by default. The generic BMI2 name also allows future BMI2 instruction optimizations to share the same deployment-level switch. ### Release note Set enable_bmi2_optimizations=false on CPUs where BMI2 optimizations are slower than scalar implementations. ### Check List (For Author) - Test: Unit Test - Compiled config.cpp and BitPackingTest objects in ASAN-UT. - Added a unit test that verifies enable_bmi2_optimizations=false disables PDEP dispatch. - Behavior changed: Yes (adds a BE configuration switch; default behavior remains enabled) - Does this need documentation: No
HappenLee
force-pushed
the
improvement-pdep-unpack
branch
from
July 17, 2026 03:23
63b42ec to
cb53beb
Compare
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
There was a problem hiding this comment.
Automated review result: request changes for two performance regressions.
Critical checkpoint conclusions:
- Goal and proof: The PR is focused on accelerating complete 32-value bit-unpack batches, and the new kernel's masks, lane ordering, exact input bounds, returned cursor, truncated tail, and unsupported-target fallback preserve scalar results. The goal is not fully met because production dispatch knowingly selects the PR-reported large-workload regressions and adds dispatch work where no full batch exists.
- Scope and parallel paths: The seven changed files are cohesive. The complete call chain through
BatchedBitReader, Parquet plain booleans, dictionary hybrid-RLE, normal materialization, and sparse external row fetching was reviewed. The two inline findings cover the only substantiated parallel-path gaps. - Concurrency and lifecycle: The kernel is stateless, adds no thread, lock, atomic, owned allocation, or cross-TU initialization dependency. The startup-loaded non-mutable BMI2 config is appropriate for fixed CPU capability and introduces no separate race or lifecycle defect.
- Error, memory, and data correctness: No ignored
Status, exception-boundary, MemTracker, ownership, bounds, lane-order, or pointer-accounting defect was found. The change does not affect transaction visibility, persistence, storage formats, data writes, FE-BE variables, or rolling-upgrade compatibility. - Configuration and compatibility: GCC/Clang x86 builds use target attributes plus runtime BMI2/AVX2 checks; non-x86, unsupported compilers, and unsupported CPUs retain scalar behavior. The default-on slow-PDEP AMD concern remains the already-known discussion
r3598211019and was not duplicated here. - Performance:
MAIN-001is blocking because a valid sparse Parquet dictionary skip can reproduce the supplied 1M-uint32_twidth-2/4/6/8 regression even on the benchmarked Intel CPU.MAIN-002removes deterministic config/feature probes from valid 8/16/24-value RLE literals that cannot execute a PDEP batch. - Tests and results: The added tests cover all widths, full batches plus a remainder, truncation, and the config opt-out on capable x86; the direct benchmark validates PDEP against scalar. Intrinsic execution remains capability-dependent and the truncated case could be stronger, but no separate product defect was established. Per the review contract, no local build or test was attempted. Formatter checks are green; the observed macOS BE-UT failure occurs before compilation because the runner selected JDK 25 instead of JDK 17, while auth-gated TeamCity failures/pending reruns were not treated as code evidence.
- Observability and user focus: No new logging or metrics are needed for this local stateless kernel. No additional user-provided review focus was supplied.
Review completion: complete after a main-agent full scan, two normal full-review subagents plus a separate risk-focused subagent, independent candidate verification/deduplication, and Round 2 same-state convergence. Two inline issues are submitted; every other suspicious point has a concrete dismissed or duplicate conclusion.
| // repeatable regressions for multiple high widths. Because the profitable high widths are | ||
| // CPU- and working-set-dependent, an irregular per-width allowlist would not be portable; | ||
| // use the scalar implementation conservatively instead. | ||
| return is_supported_type<T, BIT_WIDTH>() && BIT_WIDTH < 16; |
Contributor
There was a problem hiding this comment.
[P1] Keep known large low-width regressions on scalar
should_use() enables every supported width below 16, but the PR's own 1M-uint32_t results say widths 2, 4, 6, and 8 are slower on the benchmarked Intel Xeon. That working set is reachable: sparse external Parquet row fetches create singleton ranges; a large within-page gap goes through BaseDictDecoder::skip_values() and its RleBatchDecoder<uint32_t> forwards literal indices directly to UnpackBatch(), with no 65,535-row cap. Thus valid dictionary skips select a known regression even on fast-PDEP Intel, distinct from the existing AMD concern. Please gate on num_values/working set or retain scalar for combinations already shown to regress.
| #if defined(__x86_64__) && (defined(__GNUC__) || defined(__clang__)) | ||
| int64_t batches_read = 0; | ||
| if constexpr (PdepUnpack::should_use<OutType, BIT_WIDTH>()) { | ||
| if (PdepUnpack::is_supported()) { |
Contributor
There was a problem hiding this comment.
[P2] Skip feature probes when no full batch exists
For values_to_read < 32, batches_to_read is zero, yet this still loads the config and probes BMI2/AVX2 before falling through to the unchanged scalar remainder. Parquet hybrid-RLE legitimately sends 8-, 16-, and 24-value literal tails from FillLiteralBuffer() here, so this adds dispatch overhead to a hot path with no possibility of using PDEP, and the benchmark starts at 4,096 values so it does not cover it. Guard is_supported() with batches_to_read > 0 so short runs remain unchanged.
### What problem does this PR solve? Issue Number: None Related PR: apache#65738 Problem Summary: BitPacking::UnpackValues called PdepUnpack::is_supported() even when fewer than 32 values were available. Parquet hybrid-RLE can send 8-, 16-, and 24-value literal tails through this path, so these scalar-only calls unnecessarily loaded the BMI2 configuration and probed BMI2/AVX2 support. Short-circuit the feature probe when batches_to_read is zero so partial batches continue directly to the unchanged scalar remainder decoder. ### Release note None ### Check List (For Author) - Test: Unit Test / Static Analysis - Compiled be/ut_build_ASAN/test/CMakeFiles/doris_be_test.dir/util/bit_packing_test.cpp.o - Attempted ./run-be-ut.sh -j 48 --run --filter=BitPackingTest.*; stopped because the local CMake state triggered an unrelated 2,259-target full rebuild - PATH=/mnt/disk6/common/ldb_toolchain_toucan/bin:$PATH build-support/check-format.sh - build-support/run-clang-tidy.sh --build-dir be/ut_build_ASAN --files be/src/util/bit_packing.inline.h - Behavior changed: No (partial batches keep the scalar decoder while avoiding unnecessary runtime feature dispatch) - Does this need documentation: No
Contributor
Author
|
/review |
Contributor
Author
|
run buildall |
Contributor
There was a problem hiding this comment.
Automated review result: request changes for two additional blocking performance-validation issues.
The optimized bit layouts, buffer bounds, byte consumption, scalar remainder handoff, and architecture fallback are internally consistent. However, the current benchmark process leaves the new config flag zero-initialized, so direct PDEP cases skip and actual-path cases measure scalar; separately, production selects the marginal width-8 kernel for common exact-one-batch calls that the benchmark does not cover and whose published direct-kernel margin cannot pay the per-call dispatch. The inline comments contain the concrete evidence and fix direction. Existing AMD, large-working-set, and zero-batch threads were treated as known context and were not duplicated.
Critical checkpoint conclusions:
- Goal and proof: The PR aims to accelerate narrow bit unpacking. Functional parity is well covered by all-width and truncated-input tests, but the performance goal is not established for current-head benchmark execution or one/few-batch production calls; MAIN-001 and MAIN-002 block approval.
- Scope and reuse: The production change is localized to
UnpackValuesplus a focused kernel/config/benchmark/test addition. The scalar implementation is reused for unsupported types, widths, CPUs, architectures, and remainders. - Concurrency: No new worker, lock, shared mutable runtime state, or atomic protocol is introduced. The production config is immutable after startup; the unit test changes it sequentially and restores it.
- Lifecycle/static initialization: Production config initialization precedes decoder use, and a pre-init read would conservatively select scalar. The standalone benchmark omits config initialization, which is MAIN-001. No circular ownership or cross-TU initializer correctness dependency remains.
- Configuration: A non-dynamic deployment switch is appropriate for immutable CPU characteristics, but benchmark startup must assign its registered default before using it.
- Compatibility and parallel paths: No wire, storage, symbol, FE/BE, rolling-upgrade, transaction, persistence, or data-write format changes apply. Raw unpack callers were traced through
BatchedBitReader, Parquet RLE dictionary indices, and plain booleans; dictionary-decoding APIs remain intentionally scalar and semantically unchanged. - Conditions and error handling: CPU/width/architecture guards safely retain scalar behavior, and no
Statusor exception path is added. Batch-count/width performance selection is still too broad for the demonstrated workloads. - Tests and results: The added unit tests cover all supported widths, full batches, truncated exact boundaries, remainders, and the disable flag; optimized execution remains CPU-feature-dependent. The benchmark compares scalar/direct/actual outputs but currently cannot enable the new flag and omits 32/64/128-value actual-path cases. Per the review contract, this invocation performed static/model validation and did not run builds.
- Observability: No new operational metric or log is required for this local decoding kernel; the deployment config supplies a fallback control.
- Performance and memory: The kernel adds no allocation or MemTracker concern, and large-buffer correctness is sound. The unresolved issues are invalid current benchmark dispatch and a substantiated exact-one-batch regression; existing large-working-set and slow-CPU concerns remain in their original threads.
- Other correctness: Independent randomized models and source proofs found exact scalar parity, bounded 8/9-byte loads, exactly 32 stores, correct input/output advancement, and correct remainder behavior across production and benchmark-only widths.
- User focus: No additional user-provided focus was specified; the full PR was reviewed.
Review workflow: the main risk scan, normal full reviews, risk-focused reviews, candidate deduplication, independent verification, and same-state convergence were completed on head 790ad0cafb05c0a778c779794161892b8011846f; every suspicious point has an accepted, dismissed-with-evidence, or duplicate conclusion.
| void BM_PdepUnpack(benchmark::State& state) { | ||
| const int bit_width = static_cast<int>(state.range(0)); | ||
| const int num_values = static_cast<int>(state.range(1)); | ||
| if (!PdepUnpack::is_supported()) { |
Contributor
There was a problem hiding this comment.
[P1] Initialize config before running these benchmarks
DEFINE_Bool leaves its namespace-scope storage zero-initialized; Register only records the "true" default, and config::init() is what later assigns it. benchmark_main.cpp never calls config::init(), so this predicate is always false in the benchmark process: every direct PDEP case is skipped, while every BM_ActualUnpack case silently measures the scalar fallback. Because the config guard was added after the published runs, the current benchmark can no longer reproduce the evidence used to select production dispatch. Initialize registered defaults before RunSpecifiedBenchmarks() (or explicitly enable this flag for the direct/actual cases) and verify that the actual-path cases enter PDEP on capable hardware.
| #if defined(__x86_64__) && (defined(__GNUC__) || defined(__clang__)) | ||
| int64_t batches_read = 0; | ||
| if constexpr (PdepUnpack::should_use<OutType, BIT_WIDTH>()) { | ||
| if (batches_to_read > 0 && PdepUnpack::is_supported()) { |
Contributor
There was a problem hiding this comment.
[P1] Keep marginal one-batch calls on the scalar path
The new batches_to_read > 0 guard fixes the already-reported zero-batch probe, but exact 32-value calls are a separate normal production shape: RleBatchDecoder can send one batch through its rounded direct bypass, and FillLiteralBuffer() is fixed at 32 values. For uint32_t width 8, the PR reports only 0.224 us scalar versus 0.221 us PDEP over 4,096 values—3 ns across 128 kernels, or about 0.023 ns saved per kernel before this per-call config load, two feature tests, and branch. An exact-one-batch call therefore cannot amortize the new dispatch, while the benchmark starts at 4,096 values and never measures 32/64/128-value actual-entry calls. Add those cases and retain scalar for one/few batches at marginal widths (at least width 8) unless end-to-end results show a win.
Contributor
TPC-H: Total hot run time: 30066 ms |
Contributor
TPC-DS: Total hot run time: 177757 ms |
Contributor
ClickBench: Total hot run time: 25.02 s |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What problem does this PR solve?
Issue Number: None
Related PR: None
Problem Summary:
BitPacking::UnpackValuescurrently decodes every complete 32-value batch with scalar shifts and masks. This PR adds an x86 PDEP implementation foruint8_t,uint16_t, anduint32_t, with AVX2 widening for narrowuint16_tanduint32_tvalues.The optimized functions are compiled with
target("bmi2,avx2"). Runtime dispatch uses__builtin_cpu_supports("bmi2")and__builtin_cpu_supports("avx2"); unsupported CPUs, architectures, output types, and remainder values continue to use the generic scalar implementation. Doris therefore does not require a global-mbmi2build flag.Production dispatch deliberately uses PDEP only when
BIT_WIDTH < 16. The generic PDEP implementation remains available to the benchmark for all supported widths, but repeated measurements found non-monotonic, CPU- and working-set-dependent results at 16-32 bits. A detailed code comment explains why the scalar implementation is retained for these widths instead of adding an irregular CPU-specific allowlist.The PR also adds unit tests for every supported bit width, including full batches and truncated/remainder input, plus a reproducible benchmark comparing:
BitPacking::UnpackValuesdispatch path.The benchmark covers 4K, 256K, and 1M values and validates optimized output against the scalar reference before measuring.
Benchmark results
Hardware: Intel Xeon Platinum 8457C, 48 KiB L1D and 2 MiB private L2 per core. Release build pinned to one CPU. Values below are CPU-time medians in microseconds.
L1-sized working set: 4K
uint32_tvalues, measured with 9 randomized repetitions. Each iteration touches a 16 KiB output and 1-7 KiB of packed input; including the benchmark's reference output, all buffers total 33-39 KiB and fit in the 48 KiB L1D.L2-sized working set: 256K
uint32_tvalues. The output is 1 MiB and the packed input is 64-448 KiB.The result is workload- and bit-width-dependent. PDEP is faster for all measured widths while the working set remains in L1 or L2, although the gains at widths 8 and 14 are marginal. At 1M values, the actual PDEP path is faster than the scalar path at widths 10, 12, and 14, but slower at widths 2, 4, 6, and 8.
High-width dispatch validation
Widths 16-32 were measured twice independently. The following table is the second run with 9 randomized repetitions;
scalar/PDEPgreater than 1 means PDEP is faster.The first independent run showed the same material regressions at widths 16, 25, 26, 28, and 32. The
uint16_twidth-16 boundary was also slower with PDEP at 4K, 256K, and 1M values. Some high widths improve, but the profitable set changes with the working set and has no monotonic boundary. The production path therefore conservatively retains scalar unpacking for all widths at or above 16. After this change, the actual entry point was benchmarked again: width 15 follows direct PDEP, while widths 16-32 follow the scalar path and all outputs match the scalar reference.Release note
Improve bit-packed integer decoding for bit widths below 16 on BMI2 and AVX2 capable x86 CPUs.
Check List (For Author)
./run-be-ut.sh -j 48 --run --filter=BitPackingTest.*benchmark_test; ran scalar, direct PDEP, and actual-path cases at 4K, 256K, and 1M valuesbuild-support/check-format.shbuild-support/run-clang-tidy.sh --build-dir be/build_Release --files be/benchmark/benchmark_main.cpp be/test/util/bit_packing_test.cppBIT_WIDTH < 16; all other cases retain the scalar path.