github-actions[bot] commented on code in PR #65635:
URL: https://github.com/apache/doris/pull/65635#discussion_r3586019151
##########
be/src/exec/common/hash_table/hash_table_set_build.h:
##########
@@ -41,11 +42,15 @@ struct HashTableBuild {
};
auto creator_for_null_key = [&](auto& mapped) { mapped = {k}; };
- for (; k < _rows; ++k) {
- if (k % CHECK_FRECUENCY == 0) {
- RETURN_IF_CANCELLED(_state);
+ RETURN_IF_CANCELLED(_state);
+ // The batch path regresses StringRef keys, including serialized and
nullable strings.
+ if constexpr (std::is_same_v<typename HashTableContext::Key,
StringRef>) {
Review Comment:
The existing cancellation thread covers the new non-string
`lazy_emplace_batch_void` branch, but the `StringRef` fallback here has the
same loss of responsiveness. Before this PR the build loop checked
`RETURN_IF_CANCELLED` every `CHECK_FRECUENCY` rows for all key types; now
string/serialized/nullable-string keys check only once at line 45 and then run
this whole loop. Since the set sink has already merged the full build side
before `_process_build_block`, a cancelled query with a large string build
input can still spend a long time inserting rows. Please restore the periodic
cancellation check in this fallback loop as well, not only in the batch helper.
##########
be/benchmark/benchmark_set_hash_table_inline.hpp:
##########
@@ -0,0 +1,421 @@
+// 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
+
+#include <benchmark/benchmark.h>
+
+#include <array>
+#include <cstddef>
+#include <cstdint>
+#include <cstring>
+#include <memory>
+#include <optional>
+#include <type_traits>
+#include <utility>
+#include <vector>
+
+#include "common/compiler_util.h"
+#include "core/column/column_nullable.h"
+#include "core/column/column_vector.h"
+#include "exec/common/set_utils.h"
+
+namespace doris {
+
+namespace {
+
+enum class SetHashTableInlineOperation {
+ BUILD_COLD,
+ BUILD_ALL_HIT,
+ PROBE_ALL_HIT,
+ PROBE_MISS_KEYS,
+ PROBE_MIXED_KEYS_50_PERCENT,
+};
+
+enum class SetHashTableExecutionMode {
+ ROW,
+ // String sub-table groups are prepared outside the timed region.
+ BATCH_CORE,
+ // String sub-table grouping and hash-table operations are both timed.
+ BATCH_WITH_GROUPING,
+};
+
+template <typename Method>
+class SetHashTableMethodAdapterBase : public Method {
+public:
+ using Method::Method;
+
+protected:
+ template <typename State>
+ auto ALWAYS_INLINE find_impl(State& state, size_t i) {
+ this->template prefetch<true>(i);
+ return state.find_key_with_hash(*this->hash_table, i, this->keys[i],
this->hash_values[i]);
+ }
+
+ template <typename State, typename F, typename FF>
+ auto ALWAYS_INLINE lazy_emplace_impl(State& state, size_t i, F&& creator,
+ FF&& creator_for_null_key) {
+ this->template prefetch<false>(i);
+ return state.lazy_emplace_key(*this->hash_table, i, this->keys[i],
this->hash_values[i],
+ creator, creator_for_null_key);
+ }
+};
+
+template <typename Method>
+class SetHashTableNoInlineMethod : public
SetHashTableMethodAdapterBase<Method> {
+public:
+ using SetHashTableMethodAdapterBase<Method>::SetHashTableMethodAdapterBase;
+
+ template <typename State>
+ auto NO_INLINE find(State& state, size_t i) {
+ return this->find_impl(state, i);
+ }
+
+ template <typename State, typename F, typename FF>
+ auto NO_INLINE lazy_emplace(State& state, size_t i, F&& creator, FF&&
creator_for_null_key) {
+ return this->lazy_emplace_impl(state, i, std::forward<F>(creator),
+ std::forward<FF>(creator_for_null_key));
+ }
+};
+
+template <typename Method>
+class SetHashTableAlwaysInlineMethod : public
SetHashTableMethodAdapterBase<Method> {
+public:
+ using SetHashTableMethodAdapterBase<Method>::SetHashTableMethodAdapterBase;
+
+ template <typename State>
+ auto ALWAYS_INLINE find(State& state, size_t i) {
+ return this->find_impl(state, i);
+ }
+
+ template <typename State, typename F, typename FF>
+ auto ALWAYS_INLINE lazy_emplace(State& state, size_t i, F&& creator,
+ FF&& creator_for_null_key) {
+ return this->lazy_emplace_impl(state, i, std::forward<F>(creator),
+ std::forward<FF>(creator_for_null_key));
+ }
+};
+
+template <typename Key>
+struct SetHashTableInlineKeyStorage {
+ std::vector<Key> keys;
+
+ SetHashTableInlineKeyStorage(size_t rows, uint64_t seed) {
+ keys.reserve(rows);
+ for (size_t i = 0; i < rows; ++i) {
+ const uint64_t value = seed + i;
+ if constexpr (std::is_same_v<Key, UInt64>) {
+ keys.emplace_back(value);
+ } else if constexpr (std::is_same_v<Key, UInt104>) {
+ keys.emplace_back(UInt104 {.a = static_cast<UInt8>(value),
+ .b = static_cast<UInt32>(value >>
8),
+ .c = value *
0x9e3779b97f4a7c15ULL});
+ } else {
+ static_assert(std::is_same_v<Key, UInt64> ||
std::is_same_v<Key, UInt104>);
+ }
+ }
+ }
+};
+
+template <>
+struct SetHashTableInlineKeyStorage<StringRef> {
+ static constexpr size_t STRING_SIZE = 16;
+
+ std::vector<std::array<char, STRING_SIZE>> bytes;
+ std::vector<StringRef> keys;
+
+ SetHashTableInlineKeyStorage(size_t rows, uint64_t seed) : bytes(rows) {
+ keys.reserve(rows);
+ for (size_t i = 0; i < rows; ++i) {
+ const uint64_t value = seed + i;
+ const uint64_t mixed = value * 0x9e3779b97f4a7c15ULL;
+ std::memcpy(bytes[i].data(), &value, sizeof(value));
+ std::memcpy(bytes[i].data() + sizeof(value), &mixed,
sizeof(mixed));
+ keys.emplace_back(bytes[i].data(), STRING_SIZE);
+ }
+ }
+};
+
+template <typename Method, bool nullable>
+class SetHashTableInlineFixture {
+public:
+ using Key = typename Method::Key;
+ using State = typename Method::State;
+
+ explicit SetHashTableInlineFixture(size_t rows)
+ : _rows(rows),
+ _hit_storage(rows, 1),
+ _miss_storage(rows, rows * 4 + 1),
+ _method(create_method()) {
+ _mixed_keys.reserve(rows);
+ for (size_t i = 0; i < rows; ++i) {
+ _mixed_keys.emplace_back(i % 2 == 0 ? _hit_storage.keys[i] :
_miss_storage.keys[i]);
+ }
+
+ if constexpr (nullable) {
+ auto nested_column = ColumnInt64::create();
+ auto null_map_column = ColumnUInt8::create();
+ auto& nested_data = nested_column->get_data();
+ auto& null_map = null_map_column->get_data();
+ nested_data.reserve(rows);
+ null_map.reserve(rows);
+ for (size_t i = 0; i < rows; ++i) {
+ nested_data.push_back(static_cast<UInt64>(i + 1));
+ null_map.push_back(i % 100 == 0);
+ }
+ _nullable_column =
+ ColumnNullable::create(std::move(nested_column),
std::move(null_map_column));
+ _state_columns.push_back(_nullable_column.get());
+ }
+ _state.emplace(_state_columns);
+ select_keys(_hit_storage.keys);
+ }
+
+ void prepare_empty_table() {
+ _method->hash_table->clear_and_shrink();
+ _arena.clear();
+ _method->hash_table->reserve(_rows);
+ select_keys(_hit_storage.keys);
+ }
+
+ void prepare_filled_table(const std::vector<Key>& probe_keys) {
+ prepare_empty_table();
+ fill_table();
+ select_keys(probe_keys);
+ }
+
+ const std::vector<Key>& hit_keys() const { return _hit_storage.keys; }
+ const std::vector<Key>& miss_keys() const { return _miss_storage.keys; }
+ const std::vector<Key>& mixed_keys() const { return _mixed_keys; }
+
+ void prepare_batch_groups() {
Review Comment:
This benchmark does not actually exercise the string sub-table grouping path
it labels. `SetHashTableInlineString` is `SetMethodOneString`, which is
PHHashMap-backed, while `Method::is_string_hash_map()` only returns true for
the `StringHashMap` variants. As a result `prepare_batch_groups()` is a no-op
here, and the StringRef `BatchCore` / `BatchWithGrouping` registrations fall
through to the generic batch loop rather than timing prebuilt vs in-timed
string grouping. That makes the new benchmark results misleading for the string
batching regression this file is trying to measure; please either register a
real `StringHashMap` method for these modes or rename/remove the grouping
modes/comments for the set string context.
--
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]
