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 <[email protected]> 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 <[email protected]> Reviewed-by: Michael Smith <[email protected]> --- .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");
