Skip to content

Commit

Permalink
tidy: enable performance checks
Browse files Browse the repository at this point in the history
With the exception of performance-no-int-to-ptr, this change enables
clang tidy's performance-based lints.

The changes made are as follows, in no particular order.

- Avoid std::endl. std::endl is confusing because it also flushes the
  buffer, which is often undesirable. Instead, we should just use a newline
  character.
- When possible, declare variables as `const T&` instead of `T`, since
  it avoids a copy.
- When possible, use `std::move` to avoid a copy. However, if the type
  is trivially copyable, `std::move` does nothing and hence should be
  removed. Furthermore, if the variable to be moved is constant,
  `std::move` simply copies, and so `std::move` is confusing and should
  be removed. Finally, if the variable is moved, but the function
  parameter type is `const T&`, then a move is unnecessary, and again,
  it should be removed.
- When using `push_back` in a loop, we should reserve up front for
  better performance.
- When concatenating strings, either we should `std::move()` the
  strings, or use `string::append`. Otherwise, we pay the price of
  creating temporary strings, instead of just pushing to a single
  string.
- Trivial destructors should be declared with `= default` to allow more
  aggressive compiler optimizations.
  • Loading branch information
Riolku committed Nov 28, 2023
1 parent e216c0c commit 5e3da89
Show file tree
Hide file tree
Showing 139 changed files with 421 additions and 349 deletions.
3 changes: 3 additions & 0 deletions .clang-tidy
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,9 @@ Checks:

modernize-use-override,

performance-*,
-performance-no-int-to-ptr,

HeaderFilterRegex:
src/include|tools/benchmark/include|tools/nodejs_api/src_cpp/include|tools/python_api/src_cpp/include|tools/rust_api/include|tools/shell/include
WarningsAsErrors: "*"
8 changes: 6 additions & 2 deletions src/binder/bind/bind_ddl.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ namespace kuzu {
namespace binder {

std::vector<std::unique_ptr<Property>> Binder::bindProperties(
std::vector<std::pair<std::string, std::string>> propertyNameDataTypes) {
const std::vector<std::pair<std::string, std::string>>& propertyNameDataTypes) {
std::vector<std::unique_ptr<Property>> boundPropertyNameDataTypes;
std::unordered_set<std::string> boundPropertyNames;
boundPropertyNames.reserve(propertyNameDataTypes.size());
Expand Down Expand Up @@ -135,7 +135,11 @@ std::unique_ptr<BoundCreateTableInfo> Binder::bindCreateRelTableGroupInfo(
auto relMultiplicity = extraInfo->relMultiplicity;
std::vector<std::unique_ptr<BoundCreateTableInfo>> boundCreateRelTableInfos;
for (auto& [srcTableName, dstTableName] : extraInfo->srcDstTablePairs) {
auto relTableName = relGroupName + "_" + srcTableName + "_" + dstTableName;
auto relTableName = std::string(relGroupName)
.append("_")
.append(srcTableName)
.append("_")
.append(dstTableName);
auto relExtraInfo =
std::make_unique<ExtraCreateRelTableInfo>(relMultiplicity, srcTableName, dstTableName);
auto relCreateInfo = std::make_unique<CreateTableInfo>(
Expand Down
4 changes: 2 additions & 2 deletions src/binder/bind/bind_reading_clause.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -124,7 +124,7 @@ std::unique_ptr<BoundReadingClause> Binder::bindInQueryCall(const ReadingClause&
}
// TODO: this is dangerous because we could match to a scan function.
auto tableFunction = reinterpret_cast<function::TableFunction*>(
catalog.getBuiltInFunctions()->matchScalarFunction(std::move(funcName), inputTypes));
catalog.getBuiltInFunctions()->matchScalarFunction(funcName, inputTypes));
auto bindInput = std::make_unique<function::TableFuncBindInput>();
bindInput->inputs = std::move(inputValues);
auto bindData =
Expand All @@ -136,7 +136,7 @@ std::unique_ptr<BoundReadingClause> Binder::bindInQueryCall(const ReadingClause&
auto offset = expressionBinder.createVariableExpression(
*LogicalType::INT64(), std::string(InternalKeyword::ROW_OFFSET));
auto boundInQueryCall = std::make_unique<BoundInQueryCall>(
std::move(tableFunction), std::move(bindData), std::move(columns), offset);
tableFunction, std::move(bindData), std::move(columns), offset);
if (call.hasWherePredicate()) {
auto wherePredicate = expressionBinder.bindExpression(*call.getWherePredicate());
boundInQueryCall->setWherePredicate(std::move(wherePredicate));
Expand Down
1 change: 1 addition & 0 deletions src/binder/bind/copy/bind_copy_rdf_graph.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@ std::unique_ptr<BoundStatement> Binder::bindCopyRdfRelFrom(const Statement& /*st
columnNames.emplace_back(rdf::PID);
columnNames.emplace_back(InternalKeyword::DST_OFFSET);
std::vector<std::unique_ptr<common::LogicalType>> columnTypes;
columnTypes.reserve(3);
for (auto i = 0u; i < 3; ++i) {
columnTypes.push_back(LogicalType::INT64());
}
Expand Down
2 changes: 1 addition & 1 deletion src/binder/bind_expression/bind_comparison_expression.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ std::shared_ptr<Expression> ExpressionBinder::bindComparisonExpression(
auto child = bindExpression(*parsedExpression.getChild(i));
children.push_back(std::move(child));
}
return bindComparisonExpression(parsedExpression.getExpressionType(), std::move(children));
return bindComparisonExpression(parsedExpression.getExpressionType(), children);
}

std::shared_ptr<Expression> ExpressionBinder::bindComparisonExpression(
Expand Down
2 changes: 1 addition & 1 deletion src/binder/bind_expression/bind_function_expression.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -183,7 +183,7 @@ std::unique_ptr<Expression> ExpressionBinder::createInternalNodeIDExpression(
}

std::shared_ptr<Expression> ExpressionBinder::bindInternalIDExpression(
std::shared_ptr<Expression> expression) {
const std::shared_ptr<Expression>& expression) {
if (ExpressionUtil::isNodePattern(*expression)) {
auto& node = (NodeExpression&)*expression;
return node.getInternalID();
Expand Down
2 changes: 1 addition & 1 deletion src/binder/bind_expression/bind_property_expression.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ expression_vector ExpressionBinder::bindNodeOrRelPropertyStarExpression(const Ex
}

expression_vector ExpressionBinder::bindStructPropertyStarExpression(
std::shared_ptr<Expression> child) {
const std::shared_ptr<Expression>& child) {
expression_vector result;
auto childType = child->getDataType();
for (auto field : StructType::getFields(&childType)) {
Expand Down
9 changes: 3 additions & 6 deletions src/binder/binder.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -220,17 +220,14 @@ function::TableFunction* Binder::getScanFunction(common::FileType fileType, bool
catalog.getBuiltInFunctions()->matchScalarFunction(READ_PARQUET_FUNC_NAME, inputTypes);
} break;
case common::FileType::NPY: {
func = catalog.getBuiltInFunctions()->matchScalarFunction(
READ_NPY_FUNC_NAME, std::move(inputTypes));
func = catalog.getBuiltInFunctions()->matchScalarFunction(READ_NPY_FUNC_NAME, inputTypes);
} break;
case common::FileType::CSV: {
func = catalog.getBuiltInFunctions()->matchScalarFunction(
isParallel ? READ_CSV_PARALLEL_FUNC_NAME : READ_CSV_SERIAL_FUNC_NAME,
std::move(inputTypes));
isParallel ? READ_CSV_PARALLEL_FUNC_NAME : READ_CSV_SERIAL_FUNC_NAME, inputTypes);
} break;
case common::FileType::TURTLE: {
func = catalog.getBuiltInFunctions()->matchScalarFunction(
READ_RDF_FUNC_NAME, std::move(inputTypes));
func = catalog.getBuiltInFunctions()->matchScalarFunction(READ_RDF_FUNC_NAME, inputTypes);
} break;
default:
KU_UNREACHABLE;
Expand Down
2 changes: 1 addition & 1 deletion src/binder/bound_statement_result.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ namespace kuzu {
namespace binder {

std::unique_ptr<BoundStatementResult> BoundStatementResult::createSingleStringColumnResult(
std::string columnName) {
const std::string& columnName) {
auto result = std::make_unique<BoundStatementResult>();
auto value = std::make_unique<Value>(LogicalType{LogicalTypeID::STRING}, columnName);
auto stringColumn = std::make_shared<LiteralExpression>(std::move(value), columnName);
Expand Down
2 changes: 1 addition & 1 deletion src/binder/expression_binder.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ std::shared_ptr<Expression> ExpressionBinder::bindExpression(
}

std::shared_ptr<Expression> ExpressionBinder::foldExpression(
std::shared_ptr<Expression> expression) {
const std::shared_ptr<Expression>& expression) {
auto value = evaluator::ExpressionEvaluatorUtils::evaluateConstantExpression(
expression, binder->memoryManager);
auto result = createLiteralExpression(std::move(value));
Expand Down
14 changes: 7 additions & 7 deletions src/binder/query/query_graph.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -151,13 +151,13 @@ void QueryGraph::addQueryNode(std::shared_ptr<NodeExpression> queryNode) {
return;
}
queryNodeNameToPosMap.insert({queryNode->getUniqueName(), queryNodes.size()});
queryNodes.push_back(queryNode);
queryNodes.push_back(std::move(queryNode));
}

void QueryGraph::addQueryRel(std::shared_ptr<RelExpression> queryRel) {
KU_ASSERT(!containsQueryRel(queryRel->getUniqueName()));
queryRelNameToPosMap.insert({queryRel->getUniqueName(), queryRels.size()});
queryRels.push_back(queryRel);
queryRels.push_back(std::move(queryRel));
}

void QueryGraph::merge(const QueryGraph& other) {
Expand Down Expand Up @@ -230,8 +230,8 @@ std::unique_ptr<QueryGraphCollection> QueryGraphCollection::copy() const {
return result;
}

void PropertyKeyValCollection::addKeyVal(
std::shared_ptr<Expression> variable, const std::string& propertyName, expression_pair keyVal) {
void PropertyKeyValCollection::addKeyVal(const std::shared_ptr<Expression>& variable,
const std::string& propertyName, expression_pair keyVal) {
if (!propertyKeyValMap.contains(variable)) {
propertyKeyValMap.insert({variable, std::unordered_map<std::string, expression_pair>{}});
}
Expand All @@ -249,7 +249,7 @@ std::vector<expression_pair> PropertyKeyValCollection::getKeyVals() const {
}

std::vector<expression_pair> PropertyKeyValCollection::getKeyVals(
std::shared_ptr<Expression> variable) const {
const std::shared_ptr<Expression>& variable) const {
std::vector<expression_pair> result;
if (!propertyKeyValMap.contains(variable)) {
return result;
Expand All @@ -261,7 +261,7 @@ std::vector<expression_pair> PropertyKeyValCollection::getKeyVals(
}

bool PropertyKeyValCollection::hasKeyVal(
std::shared_ptr<Expression> variable, const std::string& propertyName) const {
const std::shared_ptr<Expression>& variable, const std::string& propertyName) const {
if (!propertyKeyValMap.contains(variable)) {
return false;
}
Expand All @@ -272,7 +272,7 @@ bool PropertyKeyValCollection::hasKeyVal(
}

expression_pair PropertyKeyValCollection::getKeyVal(
std::shared_ptr<Expression> variable, const std::string& propertyName) const {
const std::shared_ptr<Expression>& variable, const std::string& propertyName) const {
KU_ASSERT(hasKeyVal(variable, propertyName));
return propertyKeyValMap.at(variable).at(propertyName);
}
Expand Down
1 change: 1 addition & 0 deletions src/catalog/catalog_content.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -128,6 +128,7 @@ table_id_t CatalogContent::addRdfGraphSchema(const BoundCreateTableInfo& info) {

std::vector<TableSchema*> CatalogContent::getTableSchemas() const {
std::vector<TableSchema*> result;
result.reserve(tableSchemas.size());
for (auto&& [_, schema] : tableSchemas) {
result.push_back(schema.get());
}
Expand Down
2 changes: 1 addition & 1 deletion src/common/types/int128_t.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,7 @@ std::string Int128_t::ToString(int128_t input) {
break;
}
input = divModPositive(input, 10, remainder);
result = std::string(1, '0' + remainder) + result;
result = std::string(1, '0' + remainder) + std::move(result);
}
if (result.empty()) {
result = "0";
Expand Down
12 changes: 1 addition & 11 deletions src/common/types/types.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -274,10 +274,6 @@ LogicalType::LogicalType(const LogicalType& other) {
}
}

LogicalType::LogicalType(LogicalType&& other) noexcept
: typeID{other.typeID}, physicalType{other.physicalType}, extraTypeInfo{
std::move(other.extraTypeInfo)} {}

LogicalType& LogicalType::operator=(const LogicalType& other) {
typeID = other.typeID;
physicalType = other.physicalType;
Expand Down Expand Up @@ -310,13 +306,6 @@ bool LogicalType::operator!=(const LogicalType& other) const {
return !((*this) == other);
}

LogicalType& LogicalType::operator=(LogicalType&& other) noexcept {
typeID = other.typeID;
physicalType = other.physicalType;
extraTypeInfo = std::move(other.extraTypeInfo);
return *this;
}

std::string LogicalType::toString() const {
switch (typeID) {
case LogicalTypeID::MAP: {
Expand Down Expand Up @@ -660,6 +649,7 @@ std::string LogicalTypeUtils::toString(LogicalTypeID dataTypeID) {

std::string LogicalTypeUtils::toString(const std::vector<LogicalType*>& dataTypes) {
std::vector<LogicalTypeID> dataTypeIDs;
dataTypeIDs.reserve(dataTypes.size());
for (auto& dataType : dataTypes) {
dataTypeIDs.push_back(dataType->typeID);
}
Expand Down
19 changes: 12 additions & 7 deletions src/common/types/value/value.cpp
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
#include "common/types/value/value.h"

#include <utility>

#include "common/null_buffer.h"
#include "common/serializer/deserializer.h"
#include "common/serializer/serializer.h"
Expand Down Expand Up @@ -40,8 +42,8 @@ Value Value::createNullValue() {
return {};
}

Value Value::createNullValue(LogicalType dataType) {
return Value(std::move(dataType));
Value Value::createNullValue(const LogicalType& dataType) {
return Value(dataType);
}

Value Value::createDefaultValue(const LogicalType& dataType) {
Expand Down Expand Up @@ -86,7 +88,9 @@ Value Value::createDefaultValue(const LogicalType& dataType) {
case LogicalTypeID::FIXED_LIST: {
std::vector<std::unique_ptr<Value>> children;
auto childType = FixedListType::getChildType(&dataType);
for (auto i = 0u; i < FixedListType::getNumValuesInList(&dataType); ++i) {
auto listSize = FixedListType::getNumValuesInList(&dataType);
children.reserve(listSize);
for (auto i = 0u; i < listSize; ++i) {
children.push_back(std::make_unique<Value>(createDefaultValue(*childType)));
}
return Value(dataType, std::move(children));
Expand Down Expand Up @@ -206,12 +210,13 @@ Value::Value(uint8_t* val_) : isNull_{false} {
val.pointer = val_;
}

Value::Value(LogicalType type, const std::string& val_) : isNull_{false} {
Value::Value(const LogicalType& type, std::string val_) : isNull_{false} {
dataType = type.copy();
strVal = val_;
strVal = std::move(val_);
}

Value::Value(LogicalType dataType_, std::vector<std::unique_ptr<Value>> children) : isNull_{false} {
Value::Value(const LogicalType& dataType_, std::vector<std::unique_ptr<Value>> children)
: isNull_{false} {
dataType = dataType_.copy();
this->children = std::move(children);
childrenSize = this->children.size();
Expand Down Expand Up @@ -444,7 +449,7 @@ Value::Value() : isNull_{true} {
dataType = std::make_unique<LogicalType>(LogicalTypeID::ANY);
}

Value::Value(LogicalType dataType_) : isNull_{true} {
Value::Value(const LogicalType& dataType_) : isNull_{true} {
dataType = dataType_.copy();
}

Expand Down
2 changes: 1 addition & 1 deletion src/common/vector/value_vector.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ ValueVector::ValueVector(LogicalType dataType, storage::MemoryManager* memoryMan
auxiliaryBuffer = AuxiliaryBufferFactory::getAuxiliaryBuffer(this->dataType, memoryManager);
}

void ValueVector::setState(std::shared_ptr<DataChunkState> state_) {
void ValueVector::setState(const std::shared_ptr<DataChunkState>& state_) {
this->state = state_;
if (dataType.getPhysicalType() == PhysicalTypeID::STRUCT) {
auto childrenVectors = StructVector::getFieldVectors(this);
Expand Down
1 change: 1 addition & 0 deletions src/expression_evaluator/case_evaluator.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,7 @@ bool CaseExpressionEvaluator::select(SelectionVector& selVector) {

std::unique_ptr<ExpressionEvaluator> CaseExpressionEvaluator::clone() {
std::vector<std::unique_ptr<CaseAlternativeEvaluator>> clonedAlternativeEvaluators;
clonedAlternativeEvaluators.reserve(alternativeEvaluators.size());
for (auto& alternative : alternativeEvaluators) {
clonedAlternativeEvaluators.push_back(alternative->clone());
}
Expand Down
1 change: 1 addition & 0 deletions src/expression_evaluator/function_evaluator.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@ bool FunctionExpressionEvaluator::select(SelectionVector& selVector) {

std::unique_ptr<ExpressionEvaluator> FunctionExpressionEvaluator::clone() {
std::vector<std::unique_ptr<ExpressionEvaluator>> clonedChildren;
clonedChildren.reserve(children.size());
for (auto& child : children) {
clonedChildren.push_back(child->clone());
}
Expand Down
Loading

0 comments on commit 5e3da89

Please sign in to comment.