github-actions[bot] commented on code in PR #65738: URL: https://github.com/apache/doris/pull/65738#discussion_r3600303029
########## be/src/util/pdep_unpack.h: ########## @@ -0,0 +1,169 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +#pragma once + +#if defined(__x86_64__) && (defined(__GNUC__) || defined(__clang__)) + +#include <immintrin.h> + +#include <cstddef> +#include <cstdint> +#include <cstring> +#include <limits> +#include <type_traits> +#include <utility> + +#include "common/config.h" + +namespace doris { + +class PdepUnpack { +public: + static bool is_supported() { + return config::enable_bmi2_optimizations && __builtin_cpu_supports("bmi2") && + __builtin_cpu_supports("avx2"); + } + + template <typename T, int BIT_WIDTH> + static constexpr bool is_supported_type() { + return BIT_WIDTH > 0 && BIT_WIDTH <= std::numeric_limits<T>::digits && + (std::is_same_v<T, uint8_t> || std::is_same_v<T, uint16_t> || + std::is_same_v<T, uint32_t>); + } + + template <typename T, int BIT_WIDTH> + static constexpr bool should_use() { + // Keep the generic implementation available for benchmarking all supported widths, but + // only select PDEP in the production path for widths below 16. These widths can use the + // byte/word deposit layouts and the AVX2 widening specializations below. At 16 bits and + // above, unpack32() falls back to multiple generic 64-bit PDEP groups per batch and + // competes with efficient scalar specializations, including copy-like 16- and 32-bit + // cases. Benchmarks with L1-, L2-, and larger working sets show non-monotonic results and + // 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; Review 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. ########## be/src/util/bit_packing.inline.h: ########## @@ -83,8 +86,24 @@ std::pair<const uint8_t*, int64_t> BitPacking::UnpackValues(const uint8_t* __res const uint8_t* in_pos = in; OutType* out_pos = out; +#if defined(__x86_64__) && (defined(__GNUC__) || defined(__clang__)) + int64_t batches_read = 0; + if constexpr (PdepUnpack::should_use<OutType, BIT_WIDTH>()) { Review 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. -- 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]
