github-actions[bot] commented on code in PR #65738: URL: https://github.com/apache/doris/pull/65738#discussion_r3601542406
########## be/benchmark/benchmark_pdep_unpack.hpp: ########## @@ -0,0 +1,208 @@ +// 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 <benchmark/benchmark.h> + +#include <cstdint> +#include <random> +#include <vector> + +#include "util/bit_stream_utils.inline.h" +#include "util/pdep_unpack.h" + +namespace doris { +namespace { + +constexpr int kPdepUnpackBatchSize = 32; +constexpr int kPdepUnpackL1NumValues = 1 << 12; +constexpr int kPdepUnpackL2NumValues = 1 << 18; +constexpr int kPdepUnpackLargeNumValues = 1 << 20; + +template <typename T, int BIT_WIDTH> +void scalar_unpack(const uint8_t* input, int num_values, T* output) { + constexpr int bytes_per_batch = kPdepUnpackBatchSize * BIT_WIDTH / 8; + for (int i = 0; i < num_values; i += kPdepUnpackBatchSize) { + BitPacking::Unpack32Values<T, BIT_WIDTH>(input, bytes_per_batch, output + i); + input += bytes_per_batch; + } +} + +template <typename T, int BIT_WIDTH> +void pdep_unpack(const uint8_t* input, int num_values, T* output) { + for (int i = 0; i < num_values; i += kPdepUnpackBatchSize) { + PdepUnpack::unpack32<T, BIT_WIDTH>(input, output + i); + input += kPdepUnpackBatchSize * BIT_WIDTH / 8; + } +} + +template <typename T, int BIT_WIDTH, bool USE_PDEP> +void unpack_if_supported(const uint8_t* input, int num_values, T* output) { + if constexpr (PdepUnpack::is_supported_type<T, BIT_WIDTH>()) { + if constexpr (USE_PDEP) { + pdep_unpack<T, BIT_WIDTH>(input, num_values, output); + } else { + scalar_unpack<T, BIT_WIDTH>(input, num_values, output); + } + } else { + __builtin_unreachable(); + } +} + +template <typename T, bool USE_PDEP> +void unpack(int bit_width, const uint8_t* input, int num_values, T* output) { +#define UNPACK_CASE(width) \ + case width: \ + unpack_if_supported<T, width, USE_PDEP>(input, num_values, output); \ + return + switch (bit_width) { + UNPACK_CASE(1); + UNPACK_CASE(2); + UNPACK_CASE(3); + UNPACK_CASE(4); + UNPACK_CASE(5); + UNPACK_CASE(6); + UNPACK_CASE(7); + UNPACK_CASE(8); + UNPACK_CASE(9); + UNPACK_CASE(10); + UNPACK_CASE(11); + UNPACK_CASE(12); + UNPACK_CASE(13); + UNPACK_CASE(14); + UNPACK_CASE(15); + UNPACK_CASE(16); + UNPACK_CASE(17); + UNPACK_CASE(18); + UNPACK_CASE(19); + UNPACK_CASE(20); + UNPACK_CASE(21); + UNPACK_CASE(22); + UNPACK_CASE(23); + UNPACK_CASE(24); + UNPACK_CASE(25); + UNPACK_CASE(26); + UNPACK_CASE(27); + UNPACK_CASE(28); + UNPACK_CASE(29); + UNPACK_CASE(30); + UNPACK_CASE(31); + UNPACK_CASE(32); + default: + __builtin_unreachable(); + } +#undef UNPACK_CASE +} + +template <typename T> +struct PdepUnpackBenchmarkData { + PdepUnpackBenchmarkData(int bit_width, int num_values) + : input(num_values * bit_width / 8), + scalar_output(num_values), + pdep_output(num_values) { + std::mt19937_64 rng(0x17993); + for (auto& byte : input) { + byte = static_cast<uint8_t>(rng()); + } + unpack<T, false>(bit_width, input.data(), num_values, scalar_output.data()); + } + + std::vector<uint8_t> input; + std::vector<T> scalar_output; + std::vector<T> pdep_output; +}; + +template <typename T> +void BM_ScalarUnpack(benchmark::State& state) { + const int bit_width = static_cast<int>(state.range(0)); + const int num_values = static_cast<int>(state.range(1)); + PdepUnpackBenchmarkData<T> data(bit_width, num_values); + for (auto _ : state) { + unpack<T, false>(bit_width, data.input.data(), num_values, data.scalar_output.data()); + benchmark::ClobberMemory(); + } + state.SetItemsProcessed(state.iterations() * num_values); +} + +template <typename T> +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()) { Review 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. ########## 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>()) { + if (batches_to_read > 0 && PdepUnpack::is_supported()) { Review 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. -- 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]
