Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[improvement](spinlock) remove some potential bad spinlock usage (#27904) #41406

Open
wants to merge 2 commits into
base: branch-2.0
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions be/src/runtime/query_statistics.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,7 @@ QueryStatistics::~QueryStatistics() {
}

void QueryStatisticsRecvr::insert(const PQueryStatistics& statistics, int sender_id) {
std::lock_guard<SpinLock> l(_lock);
std::lock_guard<std::mutex> l(_lock);
if (!_query_statistics.contains(sender_id)) {
_query_statistics[sender_id] = std::make_shared<QueryStatistics>();
}
Expand All @@ -132,12 +132,12 @@ void QueryStatisticsRecvr::insert(const PQueryStatistics& statistics, int sender
void QueryStatisticsRecvr::insert(QueryStatisticsPtr statistics, int sender_id) {
if (!statistics->collected()) return;
if (_query_statistics.contains(sender_id)) return;
std::lock_guard<SpinLock> l(_lock);
std::lock_guard<std::mutex> l(_lock);
_query_statistics[sender_id] = statistics;
}

QueryStatisticsPtr QueryStatisticsRecvr::find(int sender_id) {
std::lock_guard<SpinLock> l(_lock);
std::lock_guard<std::mutex> l(_lock);
auto it = _query_statistics.find(sender_id);
if (it != _query_statistics.end()) {
return it->second;
Expand Down
4 changes: 2 additions & 2 deletions be/src/runtime/query_statistics.h
Original file line number Diff line number Diff line change
Expand Up @@ -155,14 +155,14 @@ class QueryStatisticsRecvr {
friend class QueryStatistics;

void merge(QueryStatistics* statistics) {
std::lock_guard<SpinLock> l(_lock);
std::lock_guard<std::mutex> l(_lock);
for (auto& pair : _query_statistics) {
statistics->merge(*(pair.second));
}
}

std::map<int, QueryStatisticsPtr> _query_statistics;
SpinLock _lock;
std::mutex _lock;
};

} // namespace doris
1 change: 0 additions & 1 deletion be/src/runtime/runtime_filter_mgr.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -319,7 +319,6 @@ Status RuntimeFilterMergeControllerEntity::merge(const PMergeFilterRequest* requ
std::to_string(request->filter_id()));
}
}
// iter->second = pair{CntlVal,SpinLock}
cntVal = iter->second.first;
{
std::lock_guard<std::mutex> l(*iter->second.second);
Expand Down
1 change: 0 additions & 1 deletion be/src/runtime/user_function_cache.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,6 @@ struct UserFunctionCacheEntry {
// used to lookup a symbol
void* lib_handle = nullptr;

SpinLock map_lock;
// from symbol_name to function pointer
std::unordered_map<std::string, void*> fptr_map;

Expand Down
36 changes: 18 additions & 18 deletions be/src/util/metrics.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,7 @@ std::string Metric::to_prometheus(const std::string& display_name, const Labels&
std::map<std::string, double> HistogramMetric::_s_output_percentiles = {
{"0.50", 50.0}, {"0.75", 75.0}, {"0.90", 90.0}, {"0.95", 95.0}, {"0.99", 99.0}};
void HistogramMetric::clear() {
std::lock_guard<SpinLock> l(_lock);
std::lock_guard<std::mutex> l(_lock);
_stats.clear();
}

Expand All @@ -136,12 +136,12 @@ void HistogramMetric::add(const uint64_t& value) {
}

void HistogramMetric::merge(const HistogramMetric& other) {
std::lock_guard<SpinLock> l(_lock);
std::lock_guard<std::mutex> l(_lock);
_stats.merge(other._stats);
}

void HistogramMetric::set_histogram(const HistogramStat& stats) {
std::lock_guard<SpinLock> l(_lock);
std::lock_guard<std::mutex> l(_lock);
_stats.clear();
_stats.merge(stats);
}
Expand Down Expand Up @@ -228,7 +228,7 @@ std::string MetricPrototype::to_prometheus(const std::string& registry_name) con
}

void MetricEntity::deregister_metric(const MetricPrototype* metric_type) {
std::lock_guard<SpinLock> l(_lock);
std::lock_guard<std::mutex> l(_lock);
auto metric = _metrics.find(metric_type);
if (metric != _metrics.end()) {
delete metric->second;
Expand All @@ -238,7 +238,7 @@ void MetricEntity::deregister_metric(const MetricPrototype* metric_type) {

Metric* MetricEntity::get_metric(const std::string& name, const std::string& group_name) const {
MetricPrototype dummy(MetricType::UNTYPED, MetricUnit::NOUNIT, name, "", group_name);
std::lock_guard<SpinLock> l(_lock);
std::lock_guard<std::mutex> l(_lock);
auto it = _metrics.find(&dummy);
if (it == _metrics.end()) {
return nullptr;
Expand All @@ -247,15 +247,15 @@ Metric* MetricEntity::get_metric(const std::string& name, const std::string& gro
}

void MetricEntity::register_hook(const std::string& name, const std::function<void()>& hook) {
std::lock_guard<SpinLock> l(_lock);
std::lock_guard<std::mutex> l(_lock);
#ifndef BE_TEST
DCHECK(_hooks.find(name) == _hooks.end()) << "hook is already exist! " << _name << ":" << name;
#endif
_hooks.emplace(name, hook);
}

void MetricEntity::deregister_hook(const std::string& name) {
std::lock_guard<SpinLock> l(_lock);
std::lock_guard<std::mutex> l(_lock);
_hooks.erase(name);
}

Expand All @@ -276,7 +276,7 @@ std::shared_ptr<MetricEntity> MetricRegistry::register_entity(const std::string&
const Labels& labels,
MetricEntityType type) {
std::shared_ptr<MetricEntity> entity = std::make_shared<MetricEntity>(type, name, labels);
std::lock_guard<SpinLock> l(_lock);
std::lock_guard<std::mutex> l(_lock);
auto inserted_entity = _entities.insert(std::make_pair(entity, 1));
if (!inserted_entity.second) {
// If exist, increase the registered count
Expand All @@ -286,7 +286,7 @@ std::shared_ptr<MetricEntity> MetricRegistry::register_entity(const std::string&
}

void MetricRegistry::deregister_entity(const std::shared_ptr<MetricEntity>& entity) {
std::lock_guard<SpinLock> l(_lock);
std::lock_guard<std::mutex> l(_lock);
auto found_entity = _entities.find(entity);
if (found_entity != _entities.end()) {
// Decrease the registered count
Expand All @@ -303,7 +303,7 @@ std::shared_ptr<MetricEntity> MetricRegistry::get_entity(const std::string& name
MetricEntityType type) {
std::shared_ptr<MetricEntity> dummy = std::make_shared<MetricEntity>(type, name, labels);

std::lock_guard<SpinLock> l(_lock);
std::lock_guard<std::mutex> l(_lock);
auto entity = _entities.find(dummy);
if (entity == _entities.end()) {
return std::shared_ptr<MetricEntity>();
Expand All @@ -312,22 +312,22 @@ std::shared_ptr<MetricEntity> MetricRegistry::get_entity(const std::string& name
}

void MetricRegistry::trigger_all_hooks(bool force) const {
std::lock_guard<SpinLock> l(_lock);
std::lock_guard<std::mutex> l(_lock);
for (const auto& entity : _entities) {
std::lock_guard<SpinLock> l(entity.first->_lock);
std::lock_guard<std::mutex> l(entity.first->_lock);
entity.first->trigger_hook_unlocked(force);
}
}

std::string MetricRegistry::to_prometheus(bool with_tablet_metrics) const {
// Reorder by MetricPrototype
EntityMetricsByType entity_metrics_by_types;
std::lock_guard<SpinLock> l(_lock);
std::lock_guard<std::mutex> l(_lock);
for (const auto& entity : _entities) {
if (entity.first->_type == MetricEntityType::kTablet && !with_tablet_metrics) {
continue;
}
std::lock_guard<SpinLock> l(entity.first->_lock);
std::lock_guard<std::mutex> l(entity.first->_lock);
entity.first->trigger_hook_unlocked(false);
for (const auto& metric : entity.first->_metrics) {
std::pair<MetricEntity*, Metric*> new_elem =
Expand Down Expand Up @@ -365,12 +365,12 @@ std::string MetricRegistry::to_prometheus(bool with_tablet_metrics) const {
std::string MetricRegistry::to_json(bool with_tablet_metrics) const {
rj::Document doc {rj::kArrayType};
rj::Document::AllocatorType& allocator = doc.GetAllocator();
std::lock_guard<SpinLock> l(_lock);
std::lock_guard<std::mutex> l(_lock);
for (const auto& entity : _entities) {
if (entity.first->_type == MetricEntityType::kTablet && !with_tablet_metrics) {
continue;
}
std::lock_guard<SpinLock> l(entity.first->_lock);
std::lock_guard<std::mutex> l(entity.first->_lock);
entity.first->trigger_hook_unlocked(false);
for (const auto& metric : entity.first->_metrics) {
rj::Value metric_obj(rj::kObjectType);
Expand Down Expand Up @@ -406,9 +406,9 @@ std::string MetricRegistry::to_json(bool with_tablet_metrics) const {

std::string MetricRegistry::to_core_string() const {
std::stringstream ss;
std::lock_guard<SpinLock> l(_lock);
std::lock_guard<std::mutex> l(_lock);
for (const auto& entity : _entities) {
std::lock_guard<SpinLock> l(entity.first->_lock);
std::lock_guard<std::mutex> l(entity.first->_lock);
entity.first->trigger_hook_unlocked(false);
for (const auto& metric : entity.first->_metrics) {
if (metric.first->is_core_metric) {
Expand Down
23 changes: 11 additions & 12 deletions be/src/util/metrics.h
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,6 @@

#include "util/core_local.h"
#include "util/histogram.h"
#include "util/spinlock.h"

namespace doris {

Expand Down Expand Up @@ -111,17 +110,17 @@ class LockSimpleMetric : public Metric {
std::string to_string() const override { return std::to_string(value()); }

T value() const {
std::lock_guard<SpinLock> l(_lock);
std::lock_guard<std::mutex> l(_lock);
return _value;
}

void increment(const T& delta) {
std::lock_guard<SpinLock> l(this->_lock);
std::lock_guard<std::mutex> l(this->_lock);
_value += delta;
}

void set_value(const T& value) {
std::lock_guard<SpinLock> l(this->_lock);
std::lock_guard<std::mutex> l(this->_lock);
_value = value;
}

Expand All @@ -130,14 +129,14 @@ class LockSimpleMetric : public Metric {
}

protected:
// We use spinlock instead of std::atomic is because atomic don't support
// We use std::mutex instead of std::atomic is because atomic don't support
// double's fetch_add
// TODO(zc): If this is atomic is bottleneck, we change to thread local.
// performance: on Intel(R) Xeon(R) CPU E5-2450 int64_t
// original type: 2ns/op
// single thread spinlock: 26ns/op
// multiple thread(8) spinlock: 2500ns/op
mutable SpinLock _lock;
// single thread std::mutex: 26ns/op
// multiple thread(8) std::mutex: 2500ns/op
mutable std::mutex _lock;
T _value;
};

Expand Down Expand Up @@ -202,7 +201,7 @@ class HistogramMetric : public Metric {

protected:
static std::map<std::string, double> _s_output_percentiles;
mutable SpinLock _lock;
mutable std::mutex _lock;
HistogramStat _stats;
};

Expand Down Expand Up @@ -351,7 +350,7 @@ class MetricEntity {

template <typename T>
Metric* register_metric(const MetricPrototype* metric_type) {
std::lock_guard<SpinLock> l(_lock);
std::lock_guard<std::mutex> l(_lock);
auto inserted_metric = _metrics.insert(std::make_pair(metric_type, nullptr));
if (inserted_metric.second) {
// If not exist, make a new metric pointer
Expand All @@ -377,7 +376,7 @@ class MetricEntity {
std::string _name;
Labels _labels;

mutable SpinLock _lock;
mutable std::mutex _lock;
MetricMap _metrics;
std::map<std::string, std::function<void()>> _hooks;
};
Expand Down Expand Up @@ -421,7 +420,7 @@ class MetricRegistry {
private:
const std::string _name;

mutable SpinLock _lock;
mutable std::mutex _lock;
// MetricEntity -> register count
std::unordered_map<std::shared_ptr<MetricEntity>, int32_t, MetricEntityHash,
MetricEntityEqualTo>
Expand Down
Loading
Loading