From 7d4e855631b72ebdca0906da0c81e749acf482cc Mon Sep 17 00:00:00 2001 From: HappenLee Date: Sat, 2 Mar 2024 23:38:57 +0800 Subject: [PATCH] [Bug](coredump) fix regresstion test coredump in multi thread access map (#31664) --- be/src/olap/comparison_predicate.h | 44 ++++++++++++++++++------------ be/src/olap/data_dir.cpp | 2 +- be/src/olap/olap_common.h | 28 +++++++++++-------- be/src/olap/snapshot_manager.cpp | 2 +- be/src/olap/storage_engine.h | 4 +-- be/src/olap/tablet.cpp | 4 +-- 6 files changed, 48 insertions(+), 36 deletions(-) diff --git a/be/src/olap/comparison_predicate.h b/be/src/olap/comparison_predicate.h index 826b1414b2afb1..3673b89a1d0077 100644 --- a/be/src/olap/comparison_predicate.h +++ b/be/src/olap/comparison_predicate.h @@ -578,24 +578,26 @@ class ComparisonPredicateBase : public ColumnPredicate { __attribute__((flatten)) int32_t _find_code_from_dictionary_column( const vectorized::ColumnDictI32& column) const { - if (!_segment_id_to_cached_code.contains(column.get_rowset_segment_id())) { - int32_t code = _is_range() ? column.find_code_by_bound(_value, _is_greater(), _is_eq()) - : column.find_code(_value); - - // Sometimes the dict is not initialized when run comparison predicate here, for example, - // the full page is null, then the reader will skip read, so that the dictionary is not - // inited. The cached code is wrong during this case, because the following page maybe not - // null, and the dict should have items in the future. - // - // Cached code may have problems, so that add a config here, if not opened, then - // we will return the code and not cache it. - if (column.is_dict_empty() || !config::enable_low_cardinality_cache_code) { - return code; - } - // If the dict is not empty, then the dict is inited and we could cache the value. - _segment_id_to_cached_code[column.get_rowset_segment_id()] = code; + int32_t code = 0; + if (_segment_id_to_cached_code.if_contains( + column.get_rowset_segment_id(), + [&code](const auto& pair) { code = pair.second; })) { + return code; + } + code = _is_range() ? column.find_code_by_bound(_value, _is_greater(), _is_eq()) + : column.find_code(_value); + // Sometimes the dict is not initialized when run comparison predicate here, for example, + // the full page is null, then the reader will skip read, so that the dictionary is not + // inited. The cached code is wrong during this case, because the following page maybe not + // null, and the dict should have items in the future. + // + // Cached code may have problems, so that add a config here, if not opened, then + // we will return the code and not cache it. + if (!column.is_dict_empty() && config::enable_low_cardinality_cache_code) { + _segment_id_to_cached_code.emplace(std::pair {column.get_rowset_segment_id(), code}); } - return _segment_id_to_cached_code[column.get_rowset_segment_id()]; + + return code; } std::string _debug_string() const override { @@ -604,7 +606,13 @@ class ComparisonPredicateBase : public ColumnPredicate { return info; } - mutable std::map, int32_t> _segment_id_to_cached_code; + mutable phmap::parallel_flat_hash_map< + std::pair, int32_t, + phmap::priv::hash_default_hash>, + phmap::priv::hash_default_eq>, + std::allocator, int32_t>>, 4, + std::shared_mutex> + _segment_id_to_cached_code; T _value; }; diff --git a/be/src/olap/data_dir.cpp b/be/src/olap/data_dir.cpp index 9a7d1ad5cdcadd..443cf1b12a3ac2 100644 --- a/be/src/olap/data_dir.cpp +++ b/be/src/olap/data_dir.cpp @@ -776,7 +776,7 @@ void DataDir::_perform_path_gc_by_rowset(const std::vector& tablet_ }; // rowset_id -> is_garbage - std::unordered_map checked_rowsets; + std::unordered_map checked_rowsets; for (auto&& [rowset_id, filename] : rowsets_not_pending) { if (auto it = checked_rowsets.find(rowset_id); it != checked_rowsets.end()) { if (it->second) { // Is checked garbage rowset diff --git a/be/src/olap/olap_common.h b/be/src/olap/olap_common.h index 42bad24dfed5b1..c08705861df7ad 100644 --- a/be/src/olap/olap_common.h +++ b/be/src/olap/olap_common.h @@ -458,18 +458,7 @@ struct RowsetId { } }; -// used for hash-struct of hash_map. -struct HashOfRowsetId { - size_t operator()(const RowsetId& rowset_id) const { - size_t seed = 0; - seed = HashUtil::hash64(&rowset_id.hi, sizeof(rowset_id.hi), seed); - seed = HashUtil::hash64(&rowset_id.mi, sizeof(rowset_id.mi), seed); - seed = HashUtil::hash64(&rowset_id.lo, sizeof(rowset_id.lo), seed); - return seed; - } -}; - -using RowsetIdUnorderedSet = std::unordered_set; +using RowsetIdUnorderedSet = std::unordered_set; // Extract rowset id from filename, return uninitialized rowset id if filename is invalid inline RowsetId extract_rowset_id(std::string_view filename) { @@ -517,3 +506,18 @@ struct RidAndPos { using PartialUpdateReadPlan = std::map>>; } // namespace doris + +// This intended to be a "good" hash function. It may change from time to time. +template <> +struct std::hash { + size_t operator()(const doris::RowsetId& rowset_id) const { + size_t seed = 0; + seed = doris::HashUtil::xxHash64WithSeed((const char*)&rowset_id.hi, sizeof(rowset_id.hi), + seed); + seed = doris::HashUtil::xxHash64WithSeed((const char*)&rowset_id.mi, sizeof(rowset_id.mi), + seed); + seed = doris::HashUtil::xxHash64WithSeed((const char*)&rowset_id.lo, sizeof(rowset_id.lo), + seed); + return seed; + } +}; diff --git a/be/src/olap/snapshot_manager.cpp b/be/src/olap/snapshot_manager.cpp index c5bf5a785db4b5..71e39e60d0f658 100644 --- a/be/src/olap/snapshot_manager.cpp +++ b/be/src/olap/snapshot_manager.cpp @@ -156,7 +156,7 @@ Result> SnapshotManager::convert_rowset_ids( tablet_schema->init_from_pb(new_tablet_meta_pb.schema()); std::unordered_map rs_version_map; - std::unordered_map rowset_id_mapping; + std::unordered_map rowset_id_mapping; guards.reserve(cloned_tablet_meta_pb.rs_metas_size() + cloned_tablet_meta_pb.stale_rs_metas_size()); for (auto&& visible_rowset : cloned_tablet_meta_pb.rs_metas()) { diff --git a/be/src/olap/storage_engine.h b/be/src/olap/storage_engine.h index 6ebfb9479f4873..b0e9ed6000e827 100644 --- a/be/src/olap/storage_engine.h +++ b/be/src/olap/storage_engine.h @@ -158,7 +158,7 @@ class BaseStorageEngine { // Hold reference of quering rowsets std::mutex _quering_rowsets_mutex; - std::unordered_map _querying_rowsets; + std::unordered_map _querying_rowsets; scoped_refptr _evict_quering_rowset_thread; }; @@ -409,7 +409,7 @@ class StorageEngine final : public BaseStorageEngine { std::atomic_bool _stopped {false}; std::mutex _gc_mutex; - std::unordered_map _unused_rowsets; + std::unordered_map _unused_rowsets; PendingRowsetSet _pending_local_rowsets; PendingRowsetSet _pending_remote_rowsets; diff --git a/be/src/olap/tablet.cpp b/be/src/olap/tablet.cpp index 7f72af7f86c9e7..c217505eaaad92 100644 --- a/be/src/olap/tablet.cpp +++ b/be/src/olap/tablet.cpp @@ -2432,10 +2432,10 @@ Status Tablet::check_rowid_conversion( return Status::OK(); } std::vector dst_segments; + RETURN_IF_ERROR( std::dynamic_pointer_cast(dst_rowset)->load_segments(&dst_segments)); - std::unordered_map, HashOfRowsetId> - input_rowsets_segment; + std::unordered_map> input_rowsets_segment; VLOG_DEBUG << "check_rowid_conversion, dst_segments size: " << dst_segments.size(); for (auto [src_rowset, locations] : location_map) {