You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@impala.apache.org by mi...@apache.org on 2023/09/08 21:53:34 UTC
[impala] 02/02: IMPALA-12390 (part 1): Enable some clang-tidy performance related checks
This is an automated email from the ASF dual-hosted git repository.
michaelsmith pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/impala.git
commit f563cce6b8eed4519772af06d89edc6e374839d3
Author: Joe McDonnell <jo...@cloudera.com>
AuthorDate: Mon Aug 21 15:42:00 2023 -0700
IMPALA-12390 (part 1): Enable some clang-tidy performance related checks
This enables several Clang Tidy performance checks and fixes
the flagged code locations. The specific checks enabled are:
1. performance-faster-string-find
"warning: 'find' called with a string literal consisting of a
single character; consider using the more effective overload
accepting a character"
Fix: Use char literals rather than string literals
2. performance-for-range-copy
"warning: loop variable is copied but only used as const reference;
consider making it a const reference"
Fix: Use const & for some locations of auto
3. performance-implicit-cast-in-loop
"warning: the type of the loop variable '$VAR' is different from
the one returned by the iterator and generates an implicit cast;
you can either change the type to the correct one ('$TYPE' but
'const auto&' is always an option) or remove the reference to make
it explicit that you are creating a new value"
Fix: Use the right type or const auto&
4. performance-inefficient-vector-operation
"warning: 'push_back' is called inside a loop; consider pre-allocating
the vector capacity before the loop"
Fix: Call reserve() on the vector before the loop
5. performance-type-promotion-in-math-fn - not encountered in our code
This disables a few performance checks temporarily to keep the
change a reasonable size.
Testing:
- Ran bin/run_clang_tidy.sh with the new checks
- Ran GVO
Change-Id: I3d5dfe04ffb4ec6f156e268c31a356651410ce91
Reviewed-on: http://gerrit.cloudera.org:8080/20387
Tested-by: Impala Public Jenkins <im...@cloudera.com>
Reviewed-by: Michael Smith <mi...@cloudera.com>
---
.clang-tidy | 6 +++++-
be/src/benchmarks/convert-timestamp-benchmark.cc | 2 ++
be/src/catalog/catalog-server.cc | 2 +-
be/src/catalog/catalog-util.cc | 6 +++---
be/src/codegen/llvm-codegen.cc | 1 +
be/src/common/logging.cc | 2 +-
be/src/exec/hdfs-columnar-scanner.cc | 1 +
be/src/exec/parquet/hdfs-parquet-scanner-test.cc | 1 +
be/src/exec/parquet/parquet-bool-decoder-test.cc | 1 +
be/src/exec/parquet/parquet-page-index-test.cc | 1 +
be/src/exprs/aggregate-functions-test.cc | 4 ++++
be/src/exprs/anyval-util.cc | 1 +
be/src/exprs/timezone_db.cc | 2 +-
be/src/rpc/authentication.cc | 2 +-
be/src/rpc/rpc-mgr.cc | 6 +++---
be/src/runtime/bufferpool/buffer-pool-test.cc | 3 ++-
be/src/runtime/bufferpool/free-list-test.cc | 1 +
be/src/runtime/coordinator.cc | 4 +++-
be/src/runtime/dml-exec-state.cc | 2 +-
be/src/runtime/io/data-cache-test.cc | 2 +-
be/src/runtime/io/disk-io-mgr-test.cc | 2 ++
be/src/runtime/io/error-converter.cc | 2 +-
be/src/runtime/tmp-file-mgr-test.cc | 4 ++--
be/src/runtime/tmp-file-mgr.cc | 2 +-
be/src/runtime/types.cc | 1 +
be/src/runtime/types.h | 4 ++--
be/src/scheduling/admission-controller-test.cc | 2 +-
be/src/scheduling/admission-controller.cc | 5 +++--
be/src/scheduling/cluster-membership-mgr.cc | 3 ++-
be/src/service/client-request-state.cc | 2 +-
be/src/service/fe-support.cc | 1 +
be/src/service/impala-http-handler.cc | 6 +++---
be/src/service/impala-server.cc | 6 +++---
be/src/statestore/statestore-subscriber.cc | 1 +
be/src/udf/uda-test.cc | 1 +
be/src/udf_samples/uda-sample-test.cc | 2 ++
be/src/util/dict-test.cc | 1 +
be/src/util/ldap-simple-bind.cc | 2 +-
be/src/util/mem-info.cc | 2 +-
be/src/util/rle-test.cc | 2 ++
be/src/util/runtime-profile.cc | 2 ++
be/src/util/simple-logger-test.cc | 4 ++--
be/src/util/string-util.cc | 2 +-
be/src/util/webserver-test.cc | 4 ++--
44 files changed, 74 insertions(+), 39 deletions(-)
diff --git a/.clang-tidy b/.clang-tidy
index 47bce4d00..fa075dec2 100644
--- a/.clang-tidy
+++ b/.clang-tidy
@@ -73,7 +73,11 @@ Checks: "-*,clang*,\
-clang-diagnostic-used-but-marked-unused,\
-clang-diagnostic-vla-extension,\
-clang-diagnostic-weak-template-vtables,\
--clang-diagnostic-weak-vtables"
+-clang-diagnostic-weak-vtables,\
+performance-*,\
+-performance-inefficient-string-concatenation,\
+-performance-unnecessary-copy-initialization,\
+-performance-unnecessary-value-param"
# Ignore warnings in gutil
diff --git a/be/src/benchmarks/convert-timestamp-benchmark.cc b/be/src/benchmarks/convert-timestamp-benchmark.cc
index 84ba26ceb..a39829ab7 100644
--- a/be/src/benchmarks/convert-timestamp-benchmark.cc
+++ b/be/src/benchmarks/convert-timestamp-benchmark.cc
@@ -177,6 +177,7 @@ vector<TimestampValue> AddTestDataDateTimes(int n, const string& startstr) {
boost::posix_time::ptime start(boost::posix_time::time_from_string(startstr));
vector<TimestampValue> data;
+ data.reserve(n);
for (int i = 0; i < n; ++i) {
start += boost::gregorian::date_duration(dis_days(gen));
start += boost::posix_time::nanoseconds(dis_nanosec(gen));
@@ -216,6 +217,7 @@ public:
const vector<FROM>& data, const char* label) {
// Create TestData for each thread.
vector<unique_ptr<TestData<FROM, TO, converter>>> test_data;
+ test_data.reserve(num_of_threads);
for (int i = 0; i < num_of_threads; ++i) {
test_data.push_back(make_unique<TestData<FROM, TO, converter>>(data));
}
diff --git a/be/src/catalog/catalog-server.cc b/be/src/catalog/catalog-server.cc
index bcbd58e1e..30ae739dd 100644
--- a/be/src/catalog/catalog-server.cc
+++ b/be/src/catalog/catalog-server.cc
@@ -919,7 +919,7 @@ void CatalogServer::TableMetricsUrlCallback(const Webserver::WebRequest& req,
if (object_name_arg != args.end()) {
// Parse the object name to extract database and table names
const string& full_tbl_name = object_name_arg->second;
- int pos = full_tbl_name.find(".");
+ int pos = full_tbl_name.find('.');
if (pos == string::npos || pos >= full_tbl_name.size() - 1) {
stringstream error_msg;
error_msg << "Invalid table name: " << full_tbl_name;
diff --git a/be/src/catalog/catalog-util.cc b/be/src/catalog/catalog-util.cc
index 3b6616ba2..26c0b5dc4 100644
--- a/be/src/catalog/catalog-util.cc
+++ b/be/src/catalog/catalog-util.cc
@@ -165,7 +165,7 @@ Status TCatalogObjectFromObjectName(const TCatalogObjectType::type& object_type,
catalog_object->__set_type(object_type);
catalog_object->__set_table(TTable());
// Parse what should be a fully qualified table name
- int pos = object_name.find(".");
+ int pos = object_name.find('.');
if (pos == string::npos || pos >= object_name.size() - 1) {
stringstream error_msg;
error_msg << "Invalid table name: " << object_name;
@@ -181,8 +181,8 @@ Status TCatalogObjectFromObjectName(const TCatalogObjectType::type& object_type,
// db, fn and signature.
catalog_object->__set_type(object_type);
catalog_object->__set_fn(TFunction());
- int dot = object_name.find(".");
- int paren = object_name.find("(");
+ int dot = object_name.find('.');
+ int paren = object_name.find('(');
if (dot == string::npos || dot >= object_name.size() - 1 ||
paren == string::npos || paren >= object_name.size() - 1 ||
paren <= dot) {
diff --git a/be/src/codegen/llvm-codegen.cc b/be/src/codegen/llvm-codegen.cc
index ac523da9d..b3ce78cd1 100644
--- a/be/src/codegen/llvm-codegen.cc
+++ b/be/src/codegen/llvm-codegen.cc
@@ -843,6 +843,7 @@ LlvmCodeGen::FnPrototype::FnPrototype(
llvm::Function* LlvmCodeGen::FnPrototype::GeneratePrototype(
LlvmBuilder* builder, llvm::Value** params) {
vector<llvm::Type*> arguments;
+ arguments.reserve(args_.size());
for (int i = 0; i < args_.size(); ++i) {
arguments.push_back(args_[i].type);
}
diff --git a/be/src/common/logging.cc b/be/src/common/logging.cc
index a7c04422c..825767ffa 100644
--- a/be/src/common/logging.cc
+++ b/be/src/common/logging.cc
@@ -61,7 +61,7 @@ string last_error_log_path = "";
void PrependFragment(string* s, bool* changed) {
impala::ThreadDebugInfo* tdi = impala::GetThreadDebugInfo();
if (tdi != nullptr) {
- for (auto id : { tdi->GetInstanceId(), tdi->GetQueryId() }) {
+ for (const auto& id : { tdi->GetInstanceId(), tdi->GetQueryId() }) {
if (id == ZERO_UNIQUE_ID) continue;
s->insert(0, PrintId(id) + "] ");
if (changed != nullptr) *changed = true;
diff --git a/be/src/exec/hdfs-columnar-scanner.cc b/be/src/exec/hdfs-columnar-scanner.cc
index fd741eecd..518091612 100644
--- a/be/src/exec/hdfs-columnar-scanner.cc
+++ b/be/src/exec/hdfs-columnar-scanner.cc
@@ -182,6 +182,7 @@ HdfsColumnarScanner::DivideReservationBetweenColumnsHelper(int64_t min_buffer_si
int64_t reservation_to_distribute) {
// Pair of (column index, reservation allocated).
ColumnReservations tmp_reservations;
+ tmp_reservations.reserve(col_range_lengths.size());
for (int i = 0; i < col_range_lengths.size(); ++i) tmp_reservations.emplace_back(i, 0);
// Sort in descending order of length, breaking ties by index so that larger columns
diff --git a/be/src/exec/parquet/hdfs-parquet-scanner-test.cc b/be/src/exec/parquet/hdfs-parquet-scanner-test.cc
index 5b55732c4..823c58d3c 100644
--- a/be/src/exec/parquet/hdfs-parquet-scanner-test.cc
+++ b/be/src/exec/parquet/hdfs-parquet-scanner-test.cc
@@ -100,6 +100,7 @@ TEST_F(HdfsParquetScannerTest, ComputeIdealReservation) {
// Test sum of reservations that doesn't fit in int32.
vector<int64_t> col_range_lengths;
const int64_t LARGE_NUM_RANGES = 10000;
+ col_range_lengths.reserve(LARGE_NUM_RANGES);
for (int i = 0; i < LARGE_NUM_RANGES; ++i) {
col_range_lengths.push_back(4 * MAX_BUFFER_SIZE);
}
diff --git a/be/src/exec/parquet/parquet-bool-decoder-test.cc b/be/src/exec/parquet/parquet-bool-decoder-test.cc
index ded6c242d..10dbdc18b 100644
--- a/be/src/exec/parquet/parquet-bool-decoder-test.cc
+++ b/be/src/exec/parquet/parquet-bool-decoder-test.cc
@@ -73,6 +73,7 @@ void TestSkipping(parquet::Encoding::type encoding, uint8_t* encoded_data,
TEST(ParquetBoolDecoder, TestDecodeAndSkipping) {
vector<bool> expected_data;
// Write 100 falses, 100 trues, 100 alternating falses and trues, 100 falses
+ expected_data.reserve(400);
for (int i = 0; i < 100; ++i) expected_data.push_back(false);
for (int i = 0; i < 100; ++i) expected_data.push_back(true);
for (int i = 0; i < 100; ++i) expected_data.push_back(i % 2);
diff --git a/be/src/exec/parquet/parquet-page-index-test.cc b/be/src/exec/parquet/parquet-page-index-test.cc
index f8a9a9c7b..6bf9d5f56 100644
--- a/be/src/exec/parquet/parquet-page-index-test.cc
+++ b/be/src/exec/parquet/parquet-page-index-test.cc
@@ -108,6 +108,7 @@ TEST(ParquetPageIndex, DeterminePageIndexRangesInRowGroup) {
template <typename T>
vector<string> ToStringVector(vector<T> vec) {
vector<string> result;
+ result.reserve(vec.size());
for (auto v : vec) {
result.emplace_back(string(reinterpret_cast<char*>(new T(v)), sizeof(v)));
}
diff --git a/be/src/exprs/aggregate-functions-test.cc b/be/src/exprs/aggregate-functions-test.cc
index b3f3b86f9..be061e489 100644
--- a/be/src/exprs/aggregate-functions-test.cc
+++ b/be/src/exprs/aggregate-functions-test.cc
@@ -98,6 +98,7 @@ TEST(HistogramTest, TestInt) {
// TODO: Add more deterministic test cases
{
vector<IntVal> input;
+ input.reserve(INPUT_SIZE);
for (int i = 0; i < INPUT_SIZE; ++i) input.push_back(i);
test_histogram.SetResultComparator(CheckHistogramDistribution);
StringVal max_expected_stdev = StringVal("100.0");
@@ -123,6 +124,7 @@ TEST(HistogramTest, TestDecimal) {
// All input values are x, result should be constant.
{
vector<DecimalVal> input;
+ input.reserve(INPUT_SIZE);
__int128_t val = MAX_UNSCALED_DECIMAL16;
stringstream ss;
for (int i = 0; i < INPUT_SIZE; ++i) input.push_back(DecimalVal(val));
@@ -135,6 +137,7 @@ TEST(HistogramTest, TestDecimal) {
{
vector<DecimalVal> input;
+ input.reserve(INPUT_SIZE);
for (int i = 0; i < INPUT_SIZE; ++i) input.push_back(DecimalVal(i));
test.SetResultComparator(CheckHistogramDistribution);
StringVal max_expected_stdev = StringVal("100.0");
@@ -154,6 +157,7 @@ TEST(HistogramTest, TestString) {
// All input values are x, result should be constant.
vector<StringVal> input;
+ input.reserve(INPUT_SIZE);
for (int i = 0; i < INPUT_SIZE; ++i) input.push_back(StringVal("x"));
char expected[] = "x, x, x, x, x, x, x, x, x, x, x, x, x, x, x, x, x, x, x, x, x, x, "
"x, x, x, x, x, x, x, x, x, x, x, x, x, x, x, x, x, x, x, x, x, x, x, x, x, x, x, "
diff --git a/be/src/exprs/anyval-util.cc b/be/src/exprs/anyval-util.cc
index fdff6583e..4cb9528b6 100644
--- a/be/src/exprs/anyval-util.cc
+++ b/be/src/exprs/anyval-util.cc
@@ -102,6 +102,7 @@ FunctionContext::TypeDesc AnyValUtil::ColumnTypeToTypeDesc(const ColumnType& typ
vector<FunctionContext::TypeDesc> AnyValUtil::ColumnTypesToTypeDescs(
const vector<ColumnType>& types) {
vector<FunctionContext::TypeDesc> type_descs;
+ type_descs.reserve(types.size());
for (const ColumnType& type : types) type_descs.push_back(ColumnTypeToTypeDesc(type));
return type_descs;
}
diff --git a/be/src/exprs/timezone_db.cc b/be/src/exprs/timezone_db.cc
index 56aa66a13..9ecedbe11 100644
--- a/be/src/exprs/timezone_db.cc
+++ b/be/src/exprs/timezone_db.cc
@@ -180,7 +180,7 @@ string TimezoneDatabase::LocalZoneName() {
string result;
while (timezone_file) {
getline(timezone_file, result);
- if (result.rfind("#", 0) == 0) continue;
+ if (result.rfind('#', 0) == 0) continue;
auto p = result.find("ZONE=\"");
if (p != string::npos) {
result.erase(p, p + 6);
diff --git a/be/src/rpc/authentication.cc b/be/src/rpc/authentication.cc
index 5dadd8e3a..cdc390916 100644
--- a/be/src/rpc/authentication.cc
+++ b/be/src/rpc/authentication.cc
@@ -503,7 +503,7 @@ int SaslAuthorizeInternal(sasl_conn_t* conn, void* context,
// Takes a Kerberos principal (either user/hostname@realm or user@realm)
// and returns the username part.
string GetShortUsernameFromKerberosPrincipal(const string& principal) {
- size_t end_idx = min(principal.find("/"), principal.find("@"));
+ size_t end_idx = min(principal.find('/'), principal.find('@'));
string short_user(
end_idx == string::npos || end_idx == 0 ?
principal : principal.substr(0, end_idx));
diff --git a/be/src/rpc/rpc-mgr.cc b/be/src/rpc/rpc-mgr.cc
index bcbf9d74a..727e84800 100644
--- a/be/src/rpc/rpc-mgr.cc
+++ b/be/src/rpc/rpc-mgr.cc
@@ -269,13 +269,13 @@ Status RpcMgr::StartServices() {
void RpcMgr::Join() {
if (services_started_) {
if (messenger_.get() == nullptr) return;
- for (auto service_pool : service_pools_) service_pool->Join();
+ for (const auto& service_pool : service_pools_) service_pool->Join();
}
}
void RpcMgr::Shutdown() {
if (messenger_.get() == nullptr) return;
- for (auto service_pool : service_pools_) service_pool->Shutdown();
+ for (const auto& service_pool : service_pools_) service_pool->Shutdown();
acceptor_pool_.reset();
messenger_->UnregisterAllServices();
@@ -377,7 +377,7 @@ void RpcMgr::ToJson(Document* document) {
// Add service pool metrics
Value services(kArrayType);
- for (auto service_pool : service_pools_) {
+ for (const auto& service_pool : service_pools_) {
Value service_entry(kObjectType);
service_pool->ToJson(&service_entry, document);
services.PushBack(service_entry, document->GetAllocator());
diff --git a/be/src/runtime/bufferpool/buffer-pool-test.cc b/be/src/runtime/bufferpool/buffer-pool-test.cc
index c322d026c..2348adef9 100644
--- a/be/src/runtime/bufferpool/buffer-pool-test.cc
+++ b/be/src/runtime/bufferpool/buffer-pool-test.cc
@@ -100,7 +100,7 @@ class BufferPoolTest : public ::testing::Test {
obj_pool_.Clear();
// Tests modify permissions, so make sure we can delete if they didn't clean up.
- for (string created_tmp_dir : created_tmp_dirs_) {
+ for (const string& created_tmp_dir : created_tmp_dirs_) {
chmod((created_tmp_dir + SCRATCH_SUFFIX).c_str(), S_IRWXU);
}
ASSERT_OK(FileSystemUtil::RemovePaths(created_tmp_dirs_));
@@ -408,6 +408,7 @@ class BufferPoolTest : public ::testing::Test {
// pages.
static string TmpFilePaths(vector<PageHandle>& pages) {
vector<string> paths;
+ paths.reserve(pages.size());
for (PageHandle& page : pages) {
paths.push_back(TmpFilePath(&page));
}
diff --git a/be/src/runtime/bufferpool/free-list-test.cc b/be/src/runtime/bufferpool/free-list-test.cc
index 5a025bf9f..86851a8b2 100644
--- a/be/src/runtime/bufferpool/free-list-test.cc
+++ b/be/src/runtime/bufferpool/free-list-test.cc
@@ -55,6 +55,7 @@ class FreeListTest : public ::testing::Test {
vector<const void*> GetSortedAddrs(const vector<BufferHandle>& buffers) {
vector<const void*> addrs;
+ addrs.reserve(buffers.size());
for (const BufferHandle& buffer : buffers) addrs.push_back(buffer.data());
std::sort(addrs.begin(), addrs.end());
return addrs;
diff --git a/be/src/runtime/coordinator.cc b/be/src/runtime/coordinator.cc
index 1bce78dde..90592ac2c 100644
--- a/be/src/runtime/coordinator.cc
+++ b/be/src/runtime/coordinator.cc
@@ -1167,7 +1167,7 @@ Status Coordinator::UpdateBlacklistWithAuxErrorInfo(
// the failed RPC. Only blacklist one node per ReportExecStatusRequestPB to avoid
// blacklisting nodes too aggressively. Currently, only blacklist the first node
// that contains a valid RPCErrorInfoPB object.
- for (auto aux_error : *aux_error_info) {
+ for (const auto& aux_error : *aux_error_info) {
if (aux_error.has_rpc_error_info()) {
RPCErrorInfoPB rpc_error_info = aux_error.rpc_error_info();
DCHECK(rpc_error_info.has_dest_node());
@@ -1257,6 +1257,7 @@ void Coordinator::HandleFailedExecRpcs(vector<BackendState*> failed_backend_stat
// Create an error based on the Exec RPC failure Status
vector<string> backend_addresses;
+ backend_addresses.reserve(failed_backend_states.size());
for (BackendState* backend_state : failed_backend_states) {
backend_addresses.push_back(
NetworkAddressPBToString(backend_state->krpc_impalad_address()));
@@ -1429,6 +1430,7 @@ void Coordinator::ReleaseBackendAdmissionControlResources(
parent_request_state_->admission_control_client();
DCHECK(admission_control_client != nullptr);
vector<NetworkAddressPB> host_addrs;
+ host_addrs.reserve(backend_states.size());
for (auto backend_state : backend_states) {
host_addrs.push_back(backend_state->impalad_address());
}
diff --git a/be/src/runtime/dml-exec-state.cc b/be/src/runtime/dml-exec-state.cc
index 61283d3ae..861ab4210 100644
--- a/be/src/runtime/dml-exec-state.cc
+++ b/be/src/runtime/dml-exec-state.cc
@@ -361,7 +361,7 @@ void DmlExecState::PopulatePathPermissionCache(hdfsFS fs, const string& path_str
string stripped_str;
if (scheme_end != string::npos) {
// Skip past the subsequent location:port/ prefix.
- stripped_str = path_str.substr(path_str.find("/", scheme_end + 3));
+ stripped_str = path_str.substr(path_str.find('/', scheme_end + 3));
} else {
stripped_str = path_str;
}
diff --git a/be/src/runtime/io/data-cache-test.cc b/be/src/runtime/io/data-cache-test.cc
index 8735bf518..2303d3146 100644
--- a/be/src/runtime/io/data-cache-test.cc
+++ b/be/src/runtime/io/data-cache-test.cc
@@ -733,7 +733,7 @@ TEST_P(DataCacheTest, AccessTraceAnonymization) {
ASSERT_OK(SimpleLogger::GetLogFiles(trace_dir, trace::TRACE_FILE_PREFIX,
&trace_files));
- for (string filename : trace_files) {
+ for (const string& filename : trace_files) {
trace::TraceFileIterator trace_file_iter(filename);
ASSERT_OK(trace_file_iter.Init());
while (true) {
diff --git a/be/src/runtime/io/disk-io-mgr-test.cc b/be/src/runtime/io/disk-io-mgr-test.cc
index 39aafed0e..645e0962f 100644
--- a/be/src/runtime/io/disk-io-mgr-test.cc
+++ b/be/src/runtime/io/disk-io-mgr-test.cc
@@ -1000,6 +1000,7 @@ TEST_F(DiskIoMgrTest, MemScarcity) {
unique_ptr<RequestContext> reader = io_mgr.RegisterContext();
vector<ScanRange*> ranges;
+ ranges.reserve(num_ranges);
for (int i = 0; i < num_ranges; ++i) {
ranges.push_back(InitRange(&pool_, tmp_file, 0, DATA_BYTES, 0, stat_val.st_mtime));
}
@@ -1482,6 +1483,7 @@ TEST_F(DiskIoMgrTest, SkipAllocateBuffers) {
// We should not read past the end of file.
vector<ScanRange*> ranges;
+ ranges.reserve(4);
for (int i = 0; i < 4; ++i) {
ranges.push_back(InitRange(&pool_, tmp_file, 0, len, 0, stat_val.st_mtime));
}
diff --git a/be/src/runtime/io/error-converter.cc b/be/src/runtime/io/error-converter.cc
index 6e2d0bfe0..2e8aff24c 100644
--- a/be/src/runtime/io/error-converter.cc
+++ b/be/src/runtime/io/error-converter.cc
@@ -100,7 +100,7 @@ bool ErrorConverter::IsBlacklistableError(const Status& status) {
size_t found = status.msg().msg().find("errno=");
if (found == string::npos) return false;
size_t start_pos = found + 6;
- size_t end_pos = status.msg().msg().find(",", start_pos);
+ size_t end_pos = status.msg().msg().find(',', start_pos);
string value = (end_pos != string::npos) ?
status.msg().msg().substr(start_pos, end_pos - start_pos) :
status.msg().msg().substr(start_pos);
diff --git a/be/src/runtime/tmp-file-mgr-test.cc b/be/src/runtime/tmp-file-mgr-test.cc
index 69b80c975..1ff4d300e 100644
--- a/be/src/runtime/tmp-file-mgr-test.cc
+++ b/be/src/runtime/tmp-file-mgr-test.cc
@@ -1094,7 +1094,7 @@ TEST_F(TmpFileMgrTest, TestDirectoryLimitParsingRemotePath) {
// Two types of paths, one with directory, one without.
vector<string> hdfs_paths{
test_env_->GetDefaultFsPath(""), test_env_->GetDefaultFsPath("/tmp")};
- for (string hdfs_path : hdfs_paths) {
+ for (const string& hdfs_path : hdfs_paths) {
string full_hdfs_path = hdfs_path + "/impala-scratch";
auto& dirs1 = GetTmpRemoteDir(CreateTmpFileMgr(hdfs_path + ",/tmp/local-buffer-dir"));
EXPECT_NE(nullptr, dirs1);
@@ -1166,7 +1166,7 @@ TEST_F(TmpFileMgrTest, TestDirectoryLimitParsingRemotePath) {
fake_hdfs_conn_map.insert(make_pair("s3a://fake_host/", fake_conn));
// Two types of paths, one with directory, one without.
vector<string> s3_paths{"s3a://fake_host", "s3a://fake_host/dir"};
- for (string s3_path : s3_paths) {
+ for (const string& s3_path : s3_paths) {
string full_s3_path = s3_path + "/impala-scratch";
auto& dirs13 = GetTmpRemoteDir(
CreateTmpFileMgr("/tmp/local-buffer-dir, " + s3_path, true, &fake_hdfs_conn_map));
diff --git a/be/src/runtime/tmp-file-mgr.cc b/be/src/runtime/tmp-file-mgr.cc
index ea871700c..c7039f776 100644
--- a/be/src/runtime/tmp-file-mgr.cc
+++ b/be/src/runtime/tmp-file-mgr.cc
@@ -817,7 +817,7 @@ Status TmpDirHdfs::ParsePathTokens(vector<string>& toks) {
split(toks, raw_path_, is_any_of(":"), token_compress_off);
// Only called on paths starting with `hdfs://` or `ofs://`.
DCHECK(toks.size() >= 2);
- if (toks[1].rfind("/") > 1) {
+ if (toks[1].rfind('/') > 1) {
// Contains a slash after the scheme, so port number was omitted.
toks[0] = Substitute("$0:$1", toks[0], toks[1]);
toks.erase(toks.begin()+1);
diff --git a/be/src/runtime/types.cc b/be/src/runtime/types.cc
index 5ae510ff3..cf7ba89b4 100644
--- a/be/src/runtime/types.cc
+++ b/be/src/runtime/types.cc
@@ -282,6 +282,7 @@ string ColumnType::DebugString() const {
vector<ColumnType> ColumnType::FromThrift(const vector<TColumnType>& ttypes) {
vector<ColumnType> types;
+ types.reserve(ttypes.size());
for (const TColumnType& ttype : ttypes) types.push_back(FromThrift(ttype));
return types;
}
diff --git a/be/src/runtime/types.h b/be/src/runtime/types.h
index 63a8600bb..d899e20f2 100644
--- a/be/src/runtime/types.h
+++ b/be/src/runtime/types.h
@@ -286,7 +286,7 @@ struct ColumnType {
switch (col_type.type) {
case TYPE_STRUCT: {
int struct_size = 0;
- for (ColumnType child_type : col_type.children) {
+ for (const ColumnType& child_type : col_type.children) {
struct_size += GetSlotSize(child_type);
}
return struct_size;
@@ -310,7 +310,7 @@ struct ColumnType {
switch (col_type.type) {
case TYPE_STRUCT: {
int struct_size = 0;
- for (ColumnType child_type : col_type.children) {
+ for (const ColumnType& child_type : col_type.children) {
struct_size += GetByteSize(child_type);
}
return struct_size;
diff --git a/be/src/scheduling/admission-controller-test.cc b/be/src/scheduling/admission-controller-test.cc
index f2d01ba15..9111a5367 100644
--- a/be/src/scheduling/admission-controller-test.cc
+++ b/be/src/scheduling/admission-controller-test.cc
@@ -186,7 +186,7 @@ class AdmissionControllerTest : public testing::Test {
/// Set the slots in use for all the hosts in 'host_addrs'.
void SetSlotsInUse(AdmissionController* admission_controller,
const vector<NetworkAddressPB>& host_addrs, int slots_in_use) {
- for (NetworkAddressPB host_addr : host_addrs) {
+ for (const NetworkAddressPB& host_addr : host_addrs) {
string host = NetworkAddressPBToString(host_addr);
admission_controller->host_stats_[host].slots_in_use = slots_in_use;
}
diff --git a/be/src/scheduling/admission-controller.cc b/be/src/scheduling/admission-controller.cc
index 76bdd774c..6d53d1f03 100644
--- a/be/src/scheduling/admission-controller.cc
+++ b/be/src/scheduling/admission-controller.cc
@@ -545,6 +545,7 @@ string AdmissionController::GetLogStringForTopNQueriesInPool(
// Now we are ready to report the stats.
// Prepare an index object that indicates the reporting for all elements.
std::vector<int> indices;
+ indices.reserve(items);
for (int i=0; i<items; i++) indices.emplace_back(i);
int indent = 0;
stringstream ss;
@@ -782,7 +783,7 @@ void AdmissionController::PoolStats::Dequeue(bool timed_out) {
void AdmissionController::UpdateStatsOnReleaseForBackends(const UniqueIdPB& query_id,
RunningQuery& running_query, const vector<NetworkAddressPB>& host_addrs) {
int64_t total_mem_to_release = 0;
- for (auto host_addr : host_addrs) {
+ for (const auto& host_addr : host_addrs) {
auto backend_allocation = running_query.per_backend_resources.find(host_addr);
if (backend_allocation == running_query.per_backend_resources.end()) {
// In the context of the admission control service, this may happen, eg. if a
@@ -1548,7 +1549,7 @@ void AdmissionController::ReleaseQueryBackendsLocked(const UniqueIdPB& query_id,
if (VLOG_IS_ON(2)) {
stringstream ss;
ss << "Released query backend(s) ";
- for (auto host_addr : host_addrs) ss << host_addr << " ";
+ for (const auto& host_addr : host_addrs) ss << host_addr << " ";
ss << "for query id=" << PrintId(query_id) << " "
<< GetPoolStats(running_query.request_pool)->DebugString();
VLOG(2) << ss.str();
diff --git a/be/src/scheduling/cluster-membership-mgr.cc b/be/src/scheduling/cluster-membership-mgr.cc
index 34e154273..426931a54 100644
--- a/be/src/scheduling/cluster-membership-mgr.cc
+++ b/be/src/scheduling/cluster-membership-mgr.cc
@@ -517,7 +517,7 @@ ClusterMembershipMgr::BeDescSharedPtr ClusterMembershipMgr::GetLocalBackendDescr
void ClusterMembershipMgr::NotifyListeners(SnapshotPtr snapshot) {
lock_guard<mutex> l(callback_fn_lock_);
- for (auto fn : update_callback_fns_) fn(snapshot);
+ for (const auto& fn : update_callback_fns_) fn(snapshot);
}
void ClusterMembershipMgr::SetState(const SnapshotPtr& new_state) {
@@ -695,6 +695,7 @@ void PopulateExecutorMembershipRequest(ClusterMembershipMgr::SnapshotPtr& snapsh
}
if (matching_exec_groups_found != snapshot->executor_groups.size()) {
vector<string> group_sets;
+ group_sets.reserve(exec_group_sets.size());
for (const auto& set : exec_group_sets) {
group_sets.push_back(set.exec_group_name_prefix);
}
diff --git a/be/src/service/client-request-state.cc b/be/src/service/client-request-state.cc
index 0d48c921e..d20cbdee9 100644
--- a/be/src/service/client-request-state.cc
+++ b/be/src/service/client-request-state.cc
@@ -912,7 +912,7 @@ void ClientRequestState::ExecLoadIcebergDataRequestImpl(TLoadDataResp response)
lock_guard<mutex> l(lock_);
RETURN_VOID_IF_ERROR(UpdateQueryStatus(Status(revert_err + hdfs_ret.GetDetail())));
}
- for (string src_path : response.loaded_files) {
+ for (const string& src_path : response.loaded_files) {
hdfsFS hdfs_src_conn;
hdfs_ret = HdfsFsCache::instance()->GetConnection(dst_path, &hdfs_src_conn);
if (!hdfs_ret.ok()) {
diff --git a/be/src/service/fe-support.cc b/be/src/service/fe-support.cc
index 2d2fa7dc0..89e29c323 100644
--- a/be/src/service/fe-support.cc
+++ b/be/src/service/fe-support.cc
@@ -447,6 +447,7 @@ Java_org_apache_impala_service_FeSupport_NativeLookupSymbol(
JniUtil::internal_exc_class(), nullptr);
vector<ColumnType> arg_types;
+ arg_types.reserve(lookup.arg_types.size());
for (int i = 0; i < lookup.arg_types.size(); ++i) {
arg_types.push_back(ColumnType::FromThrift(lookup.arg_types[i]));
}
diff --git a/be/src/service/impala-http-handler.cc b/be/src/service/impala-http-handler.cc
index 5a4eca3d9..b17afb383 100644
--- a/be/src/service/impala-http-handler.cc
+++ b/be/src/service/impala-http-handler.cc
@@ -792,12 +792,12 @@ void ImpalaHttpHandler::FillClientHostsInfo(
total_connections += connection_ids.size();
Value hostname(pair.first.c_str(), document->GetAllocator());
client_host_json.AddMember("hostname", hostname, document->GetAllocator());
- for (TUniqueId connection_id : connection_ids) {
+ for (const TUniqueId& connection_id : connection_ids) {
ImpalaServer::ConnectionToSessionMap::iterator it =
server_->connection_to_sessions_map_.find(connection_id);
if (it != server_->connection_to_sessions_map_.end()) {
std::set<TUniqueId> session_ids = it->second;
- for (TUniqueId session_id : session_ids) {
+ for (const TUniqueId& session_id : session_ids) {
ImpalaServer::SessionStateMap::iterator session_state_map_iterator =
server_->session_state_map_.find(session_id);
if (session_state_map_iterator != server_->session_state_map_.end()
@@ -885,7 +885,7 @@ void ImpalaHttpHandler::FillConnectionsInfo(
server_->connection_to_sessions_map_.find(connection_context->connection_id);
if (it != server_->connection_to_sessions_map_.end()) {
// Filter out invalid session
- for (TUniqueId session_id : it->second) {
+ for (const TUniqueId& session_id : it->second) {
if (server_->session_state_map_.find(session_id)
!= server_->session_state_map_.end())
valid_session_ids.insert(session_id);
diff --git a/be/src/service/impala-server.cc b/be/src/service/impala-server.cc
index 1a6863fd7..aaf4b6f12 100644
--- a/be/src/service/impala-server.cc
+++ b/be/src/service/impala-server.cc
@@ -634,7 +634,7 @@ Status ImpalaServer::PopulateAuthorizedProxyConfig(
token_compress_on);
if (proxy_config.size() > 0) {
for (const string& config: proxy_config) {
- size_t pos = config.find("=");
+ size_t pos = config.find('=');
if (pos == string::npos) {
return Status(config);
}
@@ -2060,7 +2060,7 @@ Status ImpalaServer::AuthorizeProxyUser(const string& user, const string& do_as_
// Get the short version of the user name (the user name up to the first '/' or '@')
// from the full principal name.
- size_t end_idx = min(user.find("/"), user.find("@"));
+ size_t end_idx = min(user.find('/'), user.find('@'));
// If neither are found (or are found at the beginning of the user name),
// return the username. Otherwise, return the username up to the matching character.
string short_user(
@@ -2117,7 +2117,7 @@ bool ImpalaServer::IsAuthorizedProxyUser(const string& user) {
// Get the short version of the user name (the user name up to the first '/' or '@')
// from the full principal name.
- size_t end_idx = min(user.find("/"), user.find("@"));
+ size_t end_idx = min(user.find('/'), user.find('@'));
// If neither are found (or are found at the beginning of the user name),
// return the username. Otherwise, return the username up to the matching character.
string short_user(
diff --git a/be/src/statestore/statestore-subscriber.cc b/be/src/statestore/statestore-subscriber.cc
index 2ef1ac5ee..b49e25491 100644
--- a/be/src/statestore/statestore-subscriber.cc
+++ b/be/src/statestore/statestore-subscriber.cc
@@ -696,6 +696,7 @@ Status StatestoreSubscriber::StatestoreStub::UpdateState(
// Put the updates into ascending order of topic name to match the lock acquisition
// order of TopicRegistration::update_lock.
vector<const TTopicDelta*> deltas_to_process;
+ deltas_to_process.reserve(incoming_topic_deltas.size());
for (auto& delta : incoming_topic_deltas) deltas_to_process.push_back(&delta.second);
sort(deltas_to_process.begin(), deltas_to_process.end(),
[](const TTopicDelta* left, const TTopicDelta* right) {
diff --git a/be/src/udf/uda-test.cc b/be/src/udf/uda-test.cc
index 485a7043d..137044517 100644
--- a/be/src/udf/uda-test.cc
+++ b/be/src/udf/uda-test.cc
@@ -300,6 +300,7 @@ TEST(MemTest, Basic) {
::MemTestInit, ::MemTestUpdate, ::MemTestMerge, ::MemTestSerialize,
::MemTestFinalize);
vector<BigIntVal> input;
+ input.reserve(10);
for (int i = 0; i < 10; ++i) {
input.push_back(10);
}
diff --git a/be/src/udf_samples/uda-sample-test.cc b/be/src/udf_samples/uda-sample-test.cc
index 6aacc87f8..766fe0201 100644
--- a/be/src/udf_samples/uda-sample-test.cc
+++ b/be/src/udf_samples/uda-sample-test.cc
@@ -60,6 +60,7 @@ bool TestAvg() {
test.SetIntermediateSize(16);
vector<DoubleVal> vals;
+ vals.reserve(1001);
for (int i = 0; i < 1001; ++i) {
vals.push_back(DoubleVal(i));
}
@@ -81,6 +82,7 @@ bool TestStringConcat() {
values.push_back("World");
vector<StringVal> separators;
+ separators.reserve(values.size());
for(int i = 0; i < values.size(); ++i) {
separators.push_back(",");
}
diff --git a/be/src/util/dict-test.cc b/be/src/util/dict-test.cc
index 097370740..d2cc61dc1 100644
--- a/be/src/util/dict-test.cc
+++ b/be/src/util/dict-test.cc
@@ -429,6 +429,7 @@ TEST(DictTest, TestSkippingValues) {
};
vector<int32_t> literal_values;
+ literal_values.reserve(200);
for (int i = 0; i < 200; ++i) literal_values.push_back(i);
ValidateSkipping(literal_values, literal_values, 0, 4);
ValidateSkipping(literal_values, literal_values, 0, 130);
diff --git a/be/src/util/ldap-simple-bind.cc b/be/src/util/ldap-simple-bind.cc
index 97ca6bafd..320995719 100644
--- a/be/src/util/ldap-simple-bind.cc
+++ b/be/src/util/ldap-simple-bind.cc
@@ -182,7 +182,7 @@ string LdapSimpleBind::ConstructUserDN(const string& user) {
string user_dn = user;
if (!FLAGS_ldap_domain.empty()) {
// Append @domain if there isn't already an @ in the user string.
- if (user_dn.find("@") == string::npos) {
+ if (user_dn.find('@') == string::npos) {
user_dn = Substitute("$0@$1", user_dn, FLAGS_ldap_domain);
}
} else if (!FLAGS_ldap_baseDN.empty()) {
diff --git a/be/src/util/mem-info.cc b/be/src/util/mem-info.cc
index 43cfd24ff..51c5ce3f1 100644
--- a/be/src/util/mem-info.cc
+++ b/be/src/util/mem-info.cc
@@ -130,7 +130,7 @@ MappedMemInfo MemInfo::ParseSmaps() {
size_t colon_pos = line.find(':');
if (colon_pos == string::npos) continue;
string name = line.substr(0, colon_pos);
- size_t non_space_after_colon_pos = line.find_first_not_of(" ", colon_pos + 1);
+ size_t non_space_after_colon_pos = line.find_first_not_of(' ', colon_pos + 1);
if (non_space_after_colon_pos == string::npos) continue;
// From the first non-space after the colon through the end of the string.
string value = line.substr(non_space_after_colon_pos);
diff --git a/be/src/util/rle-test.cc b/be/src/util/rle-test.cc
index d4534888b..a8a0812fb 100644
--- a/be/src/util/rle-test.cc
+++ b/be/src/util/rle-test.cc
@@ -230,6 +230,7 @@ class RleTest : public ::testing::Test {
void TestRleValues(int bit_width, int num_vals, int value = -1) {
const int64_t mod = (bit_width == 64) ? 1 : 1LL << bit_width;
vector<int> values;
+ values.reserve(num_vals);
for (int v = 0; v < num_vals; ++v) {
values.push_back((value != -1) ? value : (v % mod));
}
@@ -450,6 +451,7 @@ TEST_F(RleTest, BitWidthZeroLiteral) {
// group but flush before finishing.
TEST_F(RleTest, Flush) {
vector<int> values;
+ values.reserve(18);
for (int i = 0; i < 16; ++i) values.push_back(1);
values.push_back(0);
ValidateRle(values, 1, NULL, -1);
diff --git a/be/src/util/runtime-profile.cc b/be/src/util/runtime-profile.cc
index 75068bc68..b63cd5c99 100644
--- a/be/src/util/runtime-profile.cc
+++ b/be/src/util/runtime-profile.cc
@@ -1080,6 +1080,7 @@ void RuntimeProfile::SortChildrenByTotalTime() {
// Create a snapshot of total time values so that they don't change while we're
// sorting. Sort the <total_time, index> pairs, then reshuffle children_.
vector<pair<int64_t, int64_t>> total_times;
+ total_times.reserve(children_.size());
for (int i = 0; i < children_.size(); ++i) {
total_times.emplace_back(children_[i].first->total_time_counter()->value(), i);
}
@@ -1089,6 +1090,7 @@ void RuntimeProfile::SortChildrenByTotalTime() {
return p1.first > p2.first;
});
ChildVector new_children;
+ new_children.reserve(total_times.size());
for (const auto& p : total_times) new_children.emplace_back(children_[p.second]);
children_ = move(new_children);
}
diff --git a/be/src/util/simple-logger-test.cc b/be/src/util/simple-logger-test.cc
index cb47bf7aa..87921c65a 100644
--- a/be/src/util/simple-logger-test.cc
+++ b/be/src/util/simple-logger-test.cc
@@ -249,7 +249,7 @@ TEST_F(SimpleLoggerTest, Blast) {
&log_files);
ASSERT_OK(status);
// Debugging logging to help diagnosis for any problems.
- for (string log_file : log_files) {
+ for (const string& log_file : log_files) {
LOG(INFO) << "Log files after blast: " << log_file;
}
EXPECT_EQ(log_files.size(), MAX_LOG_FILES);
@@ -264,7 +264,7 @@ TEST_F(SimpleLoggerTest, Blast) {
// Regex to match the expect line. The parentheses are a grouping that lets us extract
// the number separately.
std::regex entry_regex = std::regex("entry_([0-9]+)");
- for (string log_file : log_files) {
+ for (const string& log_file : log_files) {
ifstream file(log_file);
string line;
while (getline(file, line)) {
diff --git a/be/src/util/string-util.cc b/be/src/util/string-util.cc
index 1c11acd60..676f01ee6 100644
--- a/be/src/util/string-util.cc
+++ b/be/src/util/string-util.cc
@@ -58,7 +58,7 @@ Status TruncateUp(const string& str, int32_t max_length, string* result) {
bool CommaSeparatedContains(const std::string& cs_list, const std::string& item) {
size_t pos = 0;
while (pos < cs_list.size()) {
- size_t comma_pos = cs_list.find(",", pos);
+ size_t comma_pos = cs_list.find(',', pos);
if (comma_pos == string::npos) return cs_list.compare(pos, string::npos, item) == 0;
if (cs_list.compare(pos, comma_pos - pos, item) == 0) return true;
pos = comma_pos + 1;
diff --git a/be/src/util/webserver-test.cc b/be/src/util/webserver-test.cc
index 2ed6d567f..824ff6ce5 100644
--- a/be/src/util/webserver-test.cc
+++ b/be/src/util/webserver-test.cc
@@ -89,7 +89,7 @@ struct HttpRequest {
if (method == "POST") {
request_stream << "Content-Length: 0\r\n";
}
- for (const std::pair<string, string>& header : headers) {
+ for (const auto& header : headers) {
request_stream << header.first << ": " << header.second << "\r\n";
}
@@ -449,7 +449,7 @@ TEST(Webserver, SslGoodTlsVersion) {
ASSERT_OK(webserver.Start());
}
- for (auto v : unsupported_versions) {
+ for (const auto& v : unsupported_versions) {
auto ssl_version = ScopedFlagSetter<string>::Make(&FLAGS_ssl_minimum_version, v);
MetricGroup metrics("webserver-test");