This is an automated email from the ASF dual-hosted git repository.
todd pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/kudu.git
The following commit(s) were added to refs/heads/master by this push:
new ca64aa2 client: micro-optimizations to reduce CPU and allocations
ca64aa2 is described below
commit ca64aa2ed33c90d7a9d2d8552d6f060b596e9c3f
Author: Todd Lipcon <[email protected]>
AuthorDate: Tue Mar 3 13:55:54 2020 -0800
client: micro-optimizations to reduce CPU and allocations
Various small hot-spots I saw while profiling TSBS workloads which do a
high number of very short scans:
- replace vector<RemoteTabletServer> with boost's small_vector in spots
in the client where we expect to usually have a very small number of
qualifying servers.
- Optimize ResourceMetrics to use a dense_hash_map instead of a
std::map, which is more CPU efficient. Additionally, key the map based
on StringPieces instead of std::strings, since in our use case we are
always using strings with eternal lifetime (coming from the protobuf
Descriptor) here.
- Avoid creating a vector<FieldDescriptor*> when updating resource
metrics. Instead iterate over the fields using index-based APIs that
don't allocate.
- Add a KuduSchema constructor taking a Schema rvalue-reference. This is
called once per scan in when setting up ScanConfiguration.
- Use make_shared for ColumnSchema shared_ptrs to avoid some atomic ops
This cuts cycles used in tcmalloc in kudu-tsdbd for one workload by about
11%.
Change-Id: I705aef9f2e57d44f387b58650279130ff329666d
Reviewed-on: http://gerrit.cloudera.org:8080/15439
Reviewed-by: Adar Dembo <[email protected]>
Tested-by: Todd Lipcon <[email protected]>
---
src/kudu/client/client-internal.cc | 7 +++++--
src/kudu/client/resource_metrics-internal.h | 14 +++++++++++++-
src/kudu/client/resource_metrics.cc | 23 ++++++++++++++++++++---
src/kudu/client/resource_metrics.h | 8 ++++++++
src/kudu/client/scanner-internal.cc | 11 +++++++----
src/kudu/client/schema.cc | 4 ++++
src/kudu/client/schema.h | 3 +++
src/kudu/common/schema.h | 4 ++--
8 files changed, 62 insertions(+), 12 deletions(-)
diff --git a/src/kudu/client/client-internal.cc
b/src/kudu/client/client-internal.cc
index d74ea00..66de143 100644
--- a/src/kudu/client/client-internal.cc
+++ b/src/kudu/client/client-internal.cc
@@ -32,6 +32,8 @@
#include <vector>
#include <boost/bind.hpp> // IWYU pragma: keep
+#include <boost/container/small_vector.hpp>
+#include <boost/container/vector.hpp>
#include <boost/function.hpp>
#include <gflags/gflags_declare.h>
#include <glog/logging.h>
@@ -74,6 +76,7 @@ DECLARE_int32(dns_resolver_max_threads_num);
DECLARE_uint32(dns_resolver_cache_capacity_mb);
DECLARE_uint32(dns_resolver_cache_ttl_sec);
+using boost::container::small_vector;
using std::map;
using std::pair;
using std::set;
@@ -226,8 +229,8 @@ RemoteTabletServer* KuduClient::Data::SelectTServer(const
scoped_refptr<RemoteTa
// TODO(wdberkeley): Eventually, the client might use the hierarchical
// structure of a location to determine proximity.
const string client_location = location();
- vector<RemoteTabletServer*> local;
- vector<RemoteTabletServer*> same_location;
+ small_vector<RemoteTabletServer*, 1> local;
+ small_vector<RemoteTabletServer*, 3> same_location;
local.reserve(filtered.size());
same_location.reserve(filtered.size());
for (RemoteTabletServer* rts : filtered) {
diff --git a/src/kudu/client/resource_metrics-internal.h
b/src/kudu/client/resource_metrics-internal.h
index d9a522d..498e292 100644
--- a/src/kudu/client/resource_metrics-internal.h
+++ b/src/kudu/client/resource_metrics-internal.h
@@ -17,11 +17,17 @@
#ifndef KUDU_CLIENT_RESOURCE_METRICS_INTERNAL_H
#define KUDU_CLIENT_RESOURCE_METRICS_INTERNAL_H
+#include <list>
#include <map>
#include <mutex>
#include <stdint.h>
+#include <set>
#include <string>
+#include <sparsehash/dense_hash_map>
+
+#include "kudu/gutil/hash/hash.h"
+#include "kudu/gutil/strings/stringpiece.h"
#include "kudu/util/locks.h"
namespace kudu {
@@ -37,14 +43,20 @@ class ResourceMetrics::Data {
std::map<std::string, int64_t> Get() const;
// Increment the given counter.
+ ATTRIBUTE_DEPRECATED("Use StringPiece variant instead")
void Increment(const std::string& name, int64_t amount);
+ // Increment the given counter. The memory referred to by 'name' must
+ // remain alive for the lifetime of this ResourceMetrics object.
+ void Increment(StringPiece name, int64_t amount);
+
// Return metric's current value.
int64_t GetMetric(const std::string& name) const;
private:
mutable simple_spinlock lock_;
- std::map<std::string, int64_t> counters_;
+ google::dense_hash_map<StringPiece, int64_t, GoodFastHash<StringPiece>>
counters_;
+ std::set<std::string> owned_strings_;
};
} // namespace client
diff --git a/src/kudu/client/resource_metrics.cc
b/src/kudu/client/resource_metrics.cc
index c340fe6..9159679 100644
--- a/src/kudu/client/resource_metrics.cc
+++ b/src/kudu/client/resource_metrics.cc
@@ -16,13 +16,19 @@
// under the License.
#include "kudu/client/resource_metrics.h"
-#include "kudu/client/resource_metrics-internal.h"
+#include <cstdint>
#include <map>
#include <mutex>
+#include <set>
#include <string>
+#include <utility>
+
+#include <sparsehash/dense_hash_map>
+#include "kudu/client/resource_metrics-internal.h"
#include "kudu/gutil/map-util.h"
+#include "kudu/gutil/strings/stringpiece.h"
namespace kudu {
@@ -49,18 +55,29 @@ int64_t ResourceMetrics::GetMetric(const std::string& name)
const {
return data_->GetMetric(name);
}
-ResourceMetrics::Data::Data() {}
+ResourceMetrics::Data::Data() {
+ counters_.set_empty_key("");
+}
ResourceMetrics::Data::~Data() {}
void ResourceMetrics::Data::Increment(const std::string& name, int64_t amount)
{
std::lock_guard<simple_spinlock> l(lock_);
+ auto it = owned_strings_.insert(name).first;
+ counters_[*it] += amount;
+}
+void ResourceMetrics::Data::Increment(StringPiece name, int64_t amount) {
+ std::lock_guard<simple_spinlock> l(lock_);
counters_[name] += amount;
}
std::map<std::string, int64_t> ResourceMetrics::Data::Get() const {
+ std::map<std::string, int64_t> ret;
std::lock_guard<simple_spinlock> l(lock_);
- return counters_;
+ for (const auto& p : counters_) {
+ ret.emplace(p.first.as_string(), p.second);
+ }
+ return ret;
}
int64_t ResourceMetrics::Data::GetMetric(const std::string& name) const {
diff --git a/src/kudu/client/resource_metrics.h
b/src/kudu/client/resource_metrics.h
index 4f31ac4..015087d 100644
--- a/src/kudu/client/resource_metrics.h
+++ b/src/kudu/client/resource_metrics.h
@@ -26,6 +26,12 @@
#include "kudu/util/kudu_export.h"
+#ifdef KUDU_HEADERS_NO_STUBS
+#include "kudu/gutil/port.h"
+#else
+#include "kudu/client/stubs.h"
+#endif
+
namespace kudu {
namespace client {
@@ -47,6 +53,7 @@ class KUDU_EXPORT ResourceMetrics {
/// @param [in] amount
/// The amount to increment the metric
/// (negative @c amount corresponds to decrementing the metric).
+ ATTRIBUTE_DEPRECATED("This function will become private in a future
release.")
void Increment(const std::string& name, int64_t amount);
/// Get current count for the specified metric.
@@ -57,6 +64,7 @@ class KUDU_EXPORT ResourceMetrics {
int64_t GetMetric(const std::string& name) const;
private:
+ friend class KuduScanner;
class KUDU_NO_EXPORT Data;
Data* data_;
};
diff --git a/src/kudu/client/scanner-internal.cc
b/src/kudu/client/scanner-internal.cc
index 717e5de..f552d4e 100644
--- a/src/kudu/client/scanner-internal.cc
+++ b/src/kudu/client/scanner-internal.cc
@@ -30,6 +30,7 @@
#include "kudu/client/client-internal.h"
#include "kudu/client/meta_cache.h"
+#include "kudu/client/resource_metrics-internal.h"
#include "kudu/client/schema.h"
#include "kudu/common/common.pb.h"
#include "kudu/common/encoded_key.h"
@@ -38,6 +39,7 @@
#include "kudu/common/schema.h"
#include "kudu/common/wire_protocol.h"
#include "kudu/gutil/port.h"
+#include "kudu/gutil/strings/stringpiece.h"
#include "kudu/gutil/strings/substitute.h"
#include "kudu/rpc/connection.h"
#include "kudu/rpc/rpc.h"
@@ -228,12 +230,13 @@ void KuduScanner::Data::UpdateResourceMetrics() {
if (last_response_.has_resource_metrics()) {
tserver::ResourceMetricsPB resource_metrics =
last_response_.resource_metrics();
const Reflection* reflection = resource_metrics.GetReflection();
- vector<const FieldDescriptor*> fields;
- reflection->ListFields(resource_metrics, &fields);
- for (const FieldDescriptor* field : fields) {
+ const google::protobuf::Descriptor* desc =
resource_metrics.GetDescriptor();
+ for (int i = 0; i < desc->field_count(); i++) {
+ const FieldDescriptor* field = desc->field(i);
if (reflection->HasField(resource_metrics, field) &&
field->cpp_type() == FieldDescriptor::CPPTYPE_INT64) {
- resource_metrics_.Increment(field->name(),
reflection->GetInt64(resource_metrics, field));
+ resource_metrics_.data_->Increment(StringPiece(field->name()),
+
reflection->GetInt64(resource_metrics, field));
}
}
}
diff --git a/src/kudu/client/schema.cc b/src/kudu/client/schema.cc
index 24cab91..9ac9f95 100644
--- a/src/kudu/client/schema.cc
+++ b/src/kudu/client/schema.cc
@@ -823,6 +823,10 @@ KuduSchema::KuduSchema(const Schema& schema)
: schema_(new Schema(schema)) {
}
+KuduSchema::KuduSchema(Schema&& schema)
+ : schema_(new Schema(schema)) {
+}
+
KuduSchema::~KuduSchema() {
delete schema_;
}
diff --git a/src/kudu/client/schema.h b/src/kudu/client/schema.h
index eb0dcb1..08e9846 100644
--- a/src/kudu/client/schema.h
+++ b/src/kudu/client/schema.h
@@ -715,6 +715,9 @@ class KUDU_EXPORT KuduSchema {
// For use by KuduSchema::FromSchema.
explicit KuduSchema(const Schema& schema);
+#if __cplusplus >= 201103
+ explicit KuduSchema(Schema&& schema);
+#endif
// Private since we don't want users to rely on the first N columns
// being the keys.
diff --git a/src/kudu/common/schema.h b/src/kudu/common/schema.h
index 5c6fd16..ab19174 100644
--- a/src/kudu/common/schema.h
+++ b/src/kudu/common/schema.h
@@ -224,14 +224,14 @@ class ColumnSchema {
: name_(std::move(name)),
type_info_(GetTypeInfo(type)),
is_nullable_(is_nullable),
- read_default_(read_default ? new Variant(type, read_default) :
nullptr),
+ read_default_(read_default ? std::make_shared<Variant>(type,
read_default) : nullptr),
attributes_(attributes),
type_attributes_(type_attributes),
comment_(std::move(comment)) {
if (write_default == read_default) {
write_default_ = read_default_;
} else if (write_default != nullptr) {
- write_default_.reset(new Variant(type, write_default));
+ write_default_ = std::make_shared<Variant>(type, write_default);
}
}
