From 5c05b57de373c42b32f9302b0ace814d98168c2b Mon Sep 17 00:00:00 2001 From: Jimmy Lu Date: Wed, 12 Jun 2024 13:08:19 -0700 Subject: [PATCH] Lazy materialization of RowType::parameters (#10144) Summary: Pull Request resolved: https://github.com/facebookincubator/velox/pull/10144 As part of optimization for reading tables with large number of columns (> 7000), the materialization of `RowType::parameters` of table/file schema costs a non-trivial amount of CPU and the parameters are never used. Making it lazy saves large amount of CPU for these tables. As a result of laziness, `RowType` copy constructor is deleted. This helps us spotting some places in code base where we copy the object unnessarily and this change cleans them up. Reviewed By: oerling Differential Revision: D58417885 --- velox/dwio/parquet/writer/Writer.cpp | 2 +- velox/examples/OpaqueType.cpp | 2 +- .../benchmarks/FilterProjectBenchmark.cpp | 2 +- velox/exec/tests/MergeTest.cpp | 2 +- velox/exec/tests/OrderByTest.cpp | 2 +- velox/exec/tests/TopNTest.cpp | 2 +- velox/exec/tests/utils/QueryAssertions.cpp | 4 +-- .../tests/utils/AggregationTestBase.cpp | 2 +- velox/type/Type.cpp | 15 ++++++++--- velox/type/Type.h | 17 ++++++++++-- velox/type/tests/TypeTest.cpp | 27 +++++++++++++++++++ velox/vector/BaseVector.cpp | 2 +- 12 files changed, 64 insertions(+), 15 deletions(-) diff --git a/velox/dwio/parquet/writer/Writer.cpp b/velox/dwio/parquet/writer/Writer.cpp index a37dea6c8e89..a0ba3a6cc6a5 100644 --- a/velox/dwio/parquet/writer/Writer.cpp +++ b/velox/dwio/parquet/writer/Writer.cpp @@ -175,7 +175,7 @@ std::shared_ptr<::arrow::Field> updateFieldNameRecursive( const Type& type, const std::string& name = "") { if (type.isRow()) { - auto rowType = type.asRow(); + auto& rowType = type.asRow(); auto newField = field->WithName(name); auto structType = std::dynamic_pointer_cast<::arrow::StructType>(newField->type()); diff --git a/velox/examples/OpaqueType.cpp b/velox/examples/OpaqueType.cpp index 2ded99ecc2c0..0c4fa637f6fe 100644 --- a/velox/examples/OpaqueType.cpp +++ b/velox/examples/OpaqueType.cpp @@ -314,7 +314,7 @@ VectorPtr evaluate( std::vector result{nullptr}; SelectivityVector rows{rowVector->size()}; - auto rowType = rowVector->type()->as(); + auto& rowType = rowVector->type()->as(); auto fieldAccessExprNode1 = std::make_shared( rowType.findChild(argName1), argName1); diff --git a/velox/exec/benchmarks/FilterProjectBenchmark.cpp b/velox/exec/benchmarks/FilterProjectBenchmark.cpp index c575ffd83cee..073c19cc4c8a 100644 --- a/velox/exec/benchmarks/FilterProjectBenchmark.cpp +++ b/velox/exec/benchmarks/FilterProjectBenchmark.cpp @@ -181,7 +181,7 @@ class FilterProjectBenchmark : public VectorTestBase { bool shareStringDicts, bool stringNulls) { assert(!rows.empty()); - auto type = rows[0]->type()->as(); + auto& type = rows[0]->type()->as(); auto numColumns = rows[0]->type()->size(); for (auto column = 0; column < numColumns; ++column) { if (type.childAt(column)->kind() == TypeKind::VARCHAR) { diff --git a/velox/exec/tests/MergeTest.cpp b/velox/exec/tests/MergeTest.cpp index 2bc8d390ed7f..743c462a3775 100644 --- a/velox/exec/tests/MergeTest.cpp +++ b/velox/exec/tests/MergeTest.cpp @@ -71,7 +71,7 @@ class MergeTest : public OperatorTestBase { const std::vector& inputVectors, const std::string& key1, const std::string& key2) { - auto rowType = inputVectors[0]->type()->asRow(); + auto& rowType = inputVectors[0]->type()->asRow(); auto sortingKeys = {rowType.getChildIdx(key1), rowType.getChildIdx(key2)}; std::vector sortOrders = { diff --git a/velox/exec/tests/OrderByTest.cpp b/velox/exec/tests/OrderByTest.cpp index 0d461cfe6dca..cf236baabfb8 100644 --- a/velox/exec/tests/OrderByTest.cpp +++ b/velox/exec/tests/OrderByTest.cpp @@ -152,7 +152,7 @@ class OrderByTest : public OperatorTestBase { const std::vector& input, const std::string& key1, const std::string& key2) { - auto rowType = input[0]->type()->asRow(); + auto& rowType = input[0]->type()->asRow(); auto keyIndices = {rowType.getChildIdx(key1), rowType.getChildIdx(key2)}; std::vector sortOrders = { diff --git a/velox/exec/tests/TopNTest.cpp b/velox/exec/tests/TopNTest.cpp index c81a2d5960ae..dc36a6c5a2b6 100644 --- a/velox/exec/tests/TopNTest.cpp +++ b/velox/exec/tests/TopNTest.cpp @@ -77,7 +77,7 @@ class TopNTest : public OperatorTestBase { const std::string& key1, const std::string& key2, int32_t limit) { - auto rowType = input[0]->type()->asRow(); + auto& rowType = input[0]->type()->asRow(); auto keyIndices = {rowType.getChildIdx(key1), rowType.getChildIdx(key2)}; auto sortOrderSqls = getSortOrderSqls(); diff --git a/velox/exec/tests/utils/QueryAssertions.cpp b/velox/exec/tests/utils/QueryAssertions.cpp index 6ea2be095fd7..60519397ebf8 100644 --- a/velox/exec/tests/utils/QueryAssertions.cpp +++ b/velox/exec/tests/utils/QueryAssertions.cpp @@ -875,7 +875,7 @@ std::vector materialize(const RowVectorPtr& vector) { std::vector rows; rows.reserve(size); - auto rowType = vector->type()->as(); + auto& rowType = vector->type()->as(); for (size_t i = 0; i < size; ++i) { auto numColumns = rowType.size(); @@ -896,7 +896,7 @@ void DuckDbQueryRunner::createTable( auto query = fmt::format("DROP TABLE IF EXISTS {}", name); execute(query); - auto rowType = data[0]->type()->as(); + auto& rowType = data[0]->type()->as(); ::duckdb::Connection con(db_); auto sql = duckdb::makeCreateTableSql(name, rowType); auto res = con.Query(sql); diff --git a/velox/functions/lib/aggregates/tests/utils/AggregationTestBase.cpp b/velox/functions/lib/aggregates/tests/utils/AggregationTestBase.cpp index 1d37e64c8c22..4fbe18ce1120 100644 --- a/velox/functions/lib/aggregates/tests/utils/AggregationTestBase.cpp +++ b/velox/functions/lib/aggregates/tests/utils/AggregationTestBase.cpp @@ -756,7 +756,7 @@ std::vector extractArgColumns( const core::CallTypedExprPtr& aggregateExpr, const RowVectorPtr& input, memory::MemoryPool* pool) { - auto type = input->type()->asRow(); + auto& type = input->type()->asRow(); std::vector columns; for (const auto& arg : aggregateExpr->inputs()) { if (auto field = core::TypedExprs::asFieldAccess(arg)) { diff --git a/velox/type/Type.cpp b/velox/type/Type.cpp index c02c93a7e616..84db02dd4abe 100644 --- a/velox/type/Type.cpp +++ b/velox/type/Type.cpp @@ -340,9 +340,7 @@ std::string namesAndTypesToString( } // namespace RowType::RowType(std::vector&& names, std::vector&& types) - : names_{std::move(names)}, - children_{std::move(types)}, - parameters_{createTypeParameters(children_)} { + : names_{std::move(names)}, children_{std::move(types)} { VELOX_CHECK_EQ( names_.size(), children_.size(), @@ -356,6 +354,17 @@ RowType::RowType(std::vector&& names, std::vector&& types) } } +RowType::~RowType() { + if (auto* parameters = parameters_.load()) { + delete parameters; + } +} + +std::unique_ptr> RowType::makeParameters() const { + return std::make_unique>( + createTypeParameters(children_)); +} + uint32_t RowType::size() const { return children_.size(); } diff --git a/velox/type/Type.h b/velox/type/Type.h index 0375061513d5..8b9150b8afe8 100644 --- a/velox/type/Type.h +++ b/velox/type/Type.h @@ -913,6 +913,8 @@ class RowType : public TypeBase { std::vector&& names, std::vector>&& types); + ~RowType() override; + uint32_t size() const override; const std::shared_ptr& childAt(uint32_t idx) const override; @@ -959,13 +961,24 @@ class RowType : public TypeBase { } const std::vector& parameters() const override { - return parameters_; + auto* parameters = parameters_.load(); + if (FOLLY_UNLIKELY(!parameters)) { + parameters = makeParameters().release(); + std::vector* oldParameters = nullptr; + if (!parameters_.compare_exchange_strong(oldParameters, parameters)) { + delete parameters; + parameters = oldParameters; + } + } + return *parameters; } private: + std::unique_ptr> makeParameters() const; + const std::vector names_; const std::vector> children_; - const std::vector parameters_; + mutable std::atomic*> parameters_{nullptr}; }; using RowTypePtr = std::shared_ptr; diff --git a/velox/type/tests/TypeTest.cpp b/velox/type/tests/TypeTest.cpp index f2e91b842d38..bc67b19124c2 100644 --- a/velox/type/tests/TypeTest.cpp +++ b/velox/type/tests/TypeTest.cpp @@ -461,6 +461,33 @@ TEST(TypeTest, emptyRow) { testTypeSerde(row); } +TEST(TypeTest, rowParametersMultiThreaded) { + std::vector names; + std::vector types; + for (int i = 0; i < 20'000; ++i) { + auto name = fmt::format("c{}", i); + names.push_back(name); + types.push_back(ROW({name}, {BIGINT()})); + } + auto type = ROW(std::move(names), std::move(types)); + constexpr int kNumThreads = 72; + const std::vector* parameters[kNumThreads]; + std::vector threads; + for (int i = 0; i < kNumThreads; ++i) { + threads.emplace_back([&, i] { parameters[i] = &type->parameters(); }); + } + for (auto& thread : threads) { + thread.join(); + } + for (int i = 1; i < kNumThreads; ++i) { + ASSERT_TRUE(parameters[i] == parameters[0]); + } + ASSERT_EQ(parameters[0]->size(), type->size()); + for (int i = 0; i < parameters[0]->size(); ++i) { + ASSERT_TRUE((*parameters[0])[i].type.get() == type->childAt(i).get()); + } +} + class Foo {}; class Bar {}; diff --git a/velox/vector/BaseVector.cpp b/velox/vector/BaseVector.cpp index 143728278abd..d6dcbbbb9411 100644 --- a/velox/vector/BaseVector.cpp +++ b/velox/vector/BaseVector.cpp @@ -300,7 +300,7 @@ VectorPtr BaseVector::createInternal( switch (kind) { case TypeKind::ROW: { std::vector children; - auto rowType = type->as(); + auto& rowType = type->as(); // Children are reserved the parent size and accessible for those rows. for (int32_t i = 0; i < rowType.size(); ++i) { children.push_back(create(rowType.childAt(i), size, pool));