tp: fix dynamic table lifetime scoping
This CL changes the lifetime of Table objects dynamic generator from
being owned by the generator to being owned by the calling class
(generally DbSqliteTable).
This change is necesary as otherwise we cannot have multiple cursors both
using the same generator - this for example happens when span joining
the counter_dur table against itself.
Change-Id: I350161717388fcbffbc04951ea59a312e97e8841
diff --git a/src/trace_processor/containers/BUILD.gn b/src/trace_processor/containers/BUILD.gn
index 32e0d28..f9f1466 100644
--- a/src/trace_processor/containers/BUILD.gn
+++ b/src/trace_processor/containers/BUILD.gn
@@ -23,6 +23,7 @@
"null_term_string_view.h",
"row_map.cc",
"row_map.h",
+ "sparse_vector.cc",
"sparse_vector.h",
"string_pool.cc",
"string_pool.h",
diff --git a/src/trace_processor/containers/sparse_vector.cc b/src/trace_processor/containers/sparse_vector.cc
new file mode 100644
index 0000000..ec78c6b
--- /dev/null
+++ b/src/trace_processor/containers/sparse_vector.cc
@@ -0,0 +1,25 @@
+/*
+ * Copyright (C) 2020 The Android Open Source Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#include "src/trace_processor/containers/sparse_vector.h"
+
+namespace perfetto {
+namespace trace_processor {
+
+SparseVectorBase::~SparseVectorBase() = default;
+
+} // namespace trace_processor
+} // namespace perfetto
diff --git a/src/trace_processor/containers/sparse_vector.h b/src/trace_processor/containers/sparse_vector.h
index 4a6a738..d90fb89 100644
--- a/src/trace_processor/containers/sparse_vector.h
+++ b/src/trace_processor/containers/sparse_vector.h
@@ -28,6 +28,13 @@
namespace perfetto {
namespace trace_processor {
+// Base class for SparseVector which allows type erasure to be implemented (e.g.
+// allows for std::unique_ptr<SparseVectorBase>).
+class SparseVectorBase {
+ public:
+ virtual ~SparseVectorBase();
+};
+
// A data structure which compactly stores a list of possibly nullable data.
//
// Internally, this class is implemented using a combination of a std::deque
@@ -36,10 +43,11 @@
// a slight cost (searching the BitVector to find the index into the std::deque)
// when looking up the data.
template <typename T>
-class SparseVector {
+class SparseVector : public SparseVectorBase {
public:
// Creates an empty SparseVector.
SparseVector() = default;
+ ~SparseVector() override = default;
SparseVector(SparseVector&&) = default;
SparseVector& operator=(SparseVector&&) noexcept = default;
diff --git a/src/trace_processor/db/column.cc b/src/trace_processor/db/column.cc
index f06b0ec..9aea8fe 100644
--- a/src/trace_processor/db/column.cc
+++ b/src/trace_processor/db/column.cc
@@ -32,6 +32,7 @@
table,
col_idx,
row_map_idx,
+ nullptr,
column.sparse_vector_) {}
Column::Column(const char* name,
@@ -40,8 +41,10 @@
Table* table,
uint32_t col_idx_in_table,
uint32_t row_map_idx,
- void* sparse_vector)
- : type_(type),
+ std::unique_ptr<SparseVectorBase> owned_sparse_vector,
+ SparseVectorBase* sparse_vector)
+ : owned_sparse_vector_(std::move(owned_sparse_vector)),
+ type_(type),
sparse_vector_(sparse_vector),
name_(name),
flags_(flags),
@@ -52,7 +55,7 @@
Column Column::IdColumn(Table* table, uint32_t col_idx, uint32_t row_map_idx) {
return Column("id", ColumnType::kId, kIdFlags, table, col_idx, row_map_idx,
- nullptr);
+ nullptr, nullptr);
}
void Column::StableSort(bool desc, std::vector<uint32_t>* idx) const {
diff --git a/src/trace_processor/db/column.h b/src/trace_processor/db/column.h
index b477e93..5d6519d 100644
--- a/src/trace_processor/db/column.h
+++ b/src/trace_processor/db/column.h
@@ -109,6 +109,22 @@
template <typename T>
Column(const char* name,
+ std::unique_ptr<SparseVector<T>> storage,
+ /* Flag */ uint32_t flags,
+ Table* table,
+ uint32_t col_idx_in_table,
+ uint32_t row_map_idx)
+ : Column(name,
+ ToColumnType<T>(),
+ flags,
+ table,
+ col_idx_in_table,
+ row_map_idx,
+ std::move(storage),
+ storage.get()) {}
+
+ template <typename T>
+ Column(const char* name,
SparseVector<T>* storage,
/* Flag */ uint32_t flags,
Table* table,
@@ -120,6 +136,7 @@
table,
col_idx_in_table,
row_map_idx,
+ nullptr,
storage) {}
// Create a Column has the same name and is backed by the same data as
@@ -405,7 +422,8 @@
Table* table,
uint32_t col_idx_in_table,
uint32_t row_map_idx,
- void* sparse_vector);
+ std::unique_ptr<SparseVectorBase> owned_sparse_vector,
+ SparseVectorBase* sparse_vector);
Column(const Column&) = delete;
Column& operator=(const Column&) = delete;
@@ -556,9 +574,14 @@
return string_pool_->Get(sparse_vector<StringPool::Id>().GetNonNull(idx));
}
+ // Only filled for columns which own the data inside them. Generally this is
+ // only true for columns which are dynamically generated at runtime.
+ // Keep this before |sparse_vector_|.
+ std::unique_ptr<SparseVectorBase> owned_sparse_vector_;
+
// type_ is used to cast sparse_vector_ to the correct type.
ColumnType type_ = ColumnType::kInt64;
- void* sparse_vector_ = nullptr;
+ SparseVectorBase* sparse_vector_ = nullptr;
const char* name_ = nullptr;
uint32_t flags_ = Flag::kNoFlag;
diff --git a/src/trace_processor/db/table.h b/src/trace_processor/db/table.h
index f25aec5..f089b70 100644
--- a/src/trace_processor/db/table.h
+++ b/src/trace_processor/db/table.h
@@ -150,14 +150,14 @@
template <typename T>
Table ExtendWithColumn(const char* name,
- SparseVector<T>* sv,
+ std::unique_ptr<SparseVector<T>> sv,
uint32_t flags) const {
PERFETTO_DCHECK(sv->size() == row_count_);
uint32_t row_map_count = static_cast<uint32_t>(row_maps_.size());
Table ret = Copy();
- ret.columns_.emplace_back(
- Column(name, sv, flags, &ret, GetColumnCount(), row_map_count));
+ ret.columns_.emplace_back(Column(name, std::move(sv), flags, &ret,
+ GetColumnCount(), row_map_count));
ret.row_maps_.emplace_back(RowMap(0, sv->size()));
return ret;
}
diff --git a/src/trace_processor/experimental_counter_dur_generator.cc b/src/trace_processor/experimental_counter_dur_generator.cc
index dedbe1f..31cdaf7 100644
--- a/src/trace_processor/experimental_counter_dur_generator.cc
+++ b/src/trace_processor/experimental_counter_dur_generator.cc
@@ -45,7 +45,7 @@
return util::OkStatus();
}
-Table* ExperimentalCounterDurGenerator::ComputeTable(
+std::unique_ptr<Table> ExperimentalCounterDurGenerator::ComputeTable(
const std::vector<Constraint>& cs,
const std::vector<Order>&) {
std::vector<Constraint> constraints;
@@ -57,15 +57,10 @@
}
Table table = counter_table_->Filter(constraints);
- // We structure the code the following way (instead of just resetting the
- // unique_ptr fields) to ensure that we don't hold onto a pointer to the
- // sparsevector in the table after freeing the sparsevector.
std::unique_ptr<SparseVector<int64_t>> dur_column(
new SparseVector<int64_t>(ComputeDurColumn(table)));
- counter_with_dur_table_.reset(new Table(table.ExtendWithColumn(
- "dur", dur_column.get(), TypedColumn<int64_t>::default_flags())));
- dur_column_ = std::move(dur_column);
- return counter_with_dur_table_.get();
+ return std::unique_ptr<Table>(new Table(table.ExtendWithColumn(
+ "dur", std::move(dur_column), TypedColumn<int64_t>::default_flags())));
}
// static
diff --git a/src/trace_processor/experimental_counter_dur_generator.h b/src/trace_processor/experimental_counter_dur_generator.h
index c96bc86..edc40be 100644
--- a/src/trace_processor/experimental_counter_dur_generator.h
+++ b/src/trace_processor/experimental_counter_dur_generator.h
@@ -34,16 +34,14 @@
std::string TableName() override;
uint32_t EstimateRowCount() override;
util::Status ValidateConstraints(const QueryConstraints&) override;
- Table* ComputeTable(const std::vector<Constraint>&,
- const std::vector<Order>&) override;
+ std::unique_ptr<Table> ComputeTable(const std::vector<Constraint>&,
+ const std::vector<Order>&) override;
// public + static for testing
static SparseVector<int64_t> ComputeDurColumn(const Table& table);
private:
const tables::CounterTable* counter_table_ = nullptr;
- std::unique_ptr<Table> counter_with_dur_table_;
- std::unique_ptr<SparseVector<int64_t>> dur_column_;
};
} // namespace trace_processor
diff --git a/src/trace_processor/experimental_flamegraph_generator.cc b/src/trace_processor/experimental_flamegraph_generator.cc
index f477386..51485a0 100644
--- a/src/trace_processor/experimental_flamegraph_generator.cc
+++ b/src/trace_processor/experimental_flamegraph_generator.cc
@@ -247,32 +247,35 @@
: util::ErrStatus("Failed to find required constraints");
}
-Table* ExperimentalFlamegraphGenerator::ComputeTable(
+std::unique_ptr<Table> ExperimentalFlamegraphGenerator::ComputeTable(
const std::vector<Constraint>& cs,
const std::vector<Order>&) {
// Get the input column values and compute the flamegraph using them.
auto values = GetInputValues(cs);
+ std::unique_ptr<tables::ExperimentalFlamegraphNodesTable> table;
if (values.profile_type == "graph") {
auto* tracker = HeapGraphTracker::GetOrCreate(context_);
- table_ = tracker->BuildFlamegraph(values.ts, values.upid);
+ table = tracker->BuildFlamegraph(values.ts, values.upid);
}
if (values.profile_type == "native") {
- table_ =
+ table =
BuildNativeFlamegraph(context_->storage.get(), values.upid, values.ts);
}
if (!values.focus_str.empty()) {
- table_ = FocusTable(context_->storage.get(), std::move(table_),
- values.focus_str);
+ table =
+ FocusTable(context_->storage.get(), std::move(table), values.focus_str);
// The pseudocolumns must be populated because as far as SQLite is concerned
// these are equality constraints.
auto focus_id =
context_->storage->InternString(base::StringView(values.focus_str));
- for (uint32_t i = 0; i < table_->row_count(); ++i) {
- table_->mutable_focus_str()->Set(i, focus_id);
+ for (uint32_t i = 0; i < table->row_count(); ++i) {
+ table->mutable_focus_str()->Set(i, focus_id);
}
}
- return table_.get();
+ // We need to explicitly std::move as clang complains about a bug in old
+ // compilers otherwise (-Wreturn-std-move-in-c++11).
+ return std::move(table);
}
Table::Schema ExperimentalFlamegraphGenerator::CreateSchema() {
diff --git a/src/trace_processor/experimental_flamegraph_generator.h b/src/trace_processor/experimental_flamegraph_generator.h
index 52170b8..177a65b 100644
--- a/src/trace_processor/experimental_flamegraph_generator.h
+++ b/src/trace_processor/experimental_flamegraph_generator.h
@@ -43,11 +43,10 @@
std::string TableName() override;
uint32_t EstimateRowCount() override;
util::Status ValidateConstraints(const QueryConstraints&) override;
- Table* ComputeTable(const std::vector<Constraint>& cs,
- const std::vector<Order>& ob) override;
+ std::unique_ptr<Table> ComputeTable(const std::vector<Constraint>& cs,
+ const std::vector<Order>& ob) override;
private:
- std::unique_ptr<tables::ExperimentalFlamegraphNodesTable> table_;
TraceProcessorContext* context_;
};
diff --git a/src/trace_processor/sqlite/db_sqlite_table.cc b/src/trace_processor/sqlite/db_sqlite_table.cc
index 95f4aa2..940d646 100644
--- a/src/trace_processor/sqlite/db_sqlite_table.cc
+++ b/src/trace_processor/sqlite/db_sqlite_table.cc
@@ -424,8 +424,9 @@
case TableComputation::kDynamic:
// If we have a dynamically created table, regenerate the table based on
// the new constraints.
- upstream_table_ =
+ dynamic_table_ =
db_sqlite_table_->generator_->ComputeTable(constraints_, orders_);
+ upstream_table_ = dynamic_table_.get();
if (!upstream_table_)
return SQLITE_CONSTRAINT;
break;
diff --git a/src/trace_processor/sqlite/db_sqlite_table.h b/src/trace_processor/sqlite/db_sqlite_table.h
index cfb5003..64231de 100644
--- a/src/trace_processor/sqlite/db_sqlite_table.h
+++ b/src/trace_processor/sqlite/db_sqlite_table.h
@@ -63,13 +63,9 @@
// Dynamically computes the table given the constraints and order by
// vectors.
- //
- // Implementations should store the generated table inside a stable pointer
- // (e.g. unique_ptr, shared_ptr or similar) and return the pointer to that
- // object. The pointer should not be freed until a successive call to
- // |ComputeTable|.
- virtual Table* ComputeTable(const std::vector<Constraint>& cs,
- const std::vector<Order>& ob) = 0;
+ virtual std::unique_ptr<Table> ComputeTable(
+ const std::vector<Constraint>& cs,
+ const std::vector<Order>& ob) = 0;
};
class Cursor : public SqliteTable::Cursor {
@@ -111,6 +107,10 @@
const Table* upstream_table_ = nullptr;
+ // Only valid for |db_sqlite_table_->computation_| ==
+ // TableComputation::kDynamic.
+ std::unique_ptr<Table> dynamic_table_;
+
// Only valid for Mode::kSingleRow.
base::Optional<uint32_t> single_row_;
