[PM] Create GraphOwned helper class.
This provides functionality that allows external clients to tie lifetimes
of their objects directly to that of a graph instance. Can be used in a
way similar to PerformanceManager::RegisterObserver, but more generically.
This will ease the transition of legacy observers to the new public
interfaces.
BUG=910288
Change-Id: I22cd06ba1a6b8a19c4bd76b05c0523c2f76ee421
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1665548
Commit-Queue: Chris Hamilton <[email protected]>
Reviewed-by: Sigurður Ásgeirsson <[email protected]>
Cr-Commit-Position: refs/heads/master@{#673114}
diff --git a/chrome/browser/performance_manager/graph/graph.cc b/chrome/browser/performance_manager/graph/graph.cc
index 27a6cf9e..ce08169 100644
--- a/chrome/browser/performance_manager/graph/graph.cc
+++ b/chrome/browser/performance_manager/graph/graph.cc
@@ -12,4 +12,10 @@
GraphObserver::GraphObserver() = default;
GraphObserver::~GraphObserver() = default;
+GraphOwned::GraphOwned() = default;
+GraphOwned::~GraphOwned() = default;
+
+GraphOwnedDefaultImpl::GraphOwnedDefaultImpl() = default;
+GraphOwnedDefaultImpl::~GraphOwnedDefaultImpl() = default;
+
} // namespace performance_manager
diff --git a/chrome/browser/performance_manager/graph/graph_impl.cc b/chrome/browser/performance_manager/graph/graph_impl.cc
index 8c397fdb..858951e4 100644
--- a/chrome/browser/performance_manager/graph/graph_impl.cc
+++ b/chrome/browser/performance_manager/graph/graph_impl.cc
@@ -61,6 +61,8 @@
DETACH_FROM_SEQUENCE(sequence_checker_);
}
+// TODO(chrisha|siggi): Move this to a TearDown, which is invoked by the
+// performance manager before the graph destructor.
GraphImpl::~GraphImpl() {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
@@ -68,6 +70,18 @@
for (auto* observer : graph_observers_)
observer->OnBeforeGraphDestroyed(this);
+ // Clean up graph owned objects. This causes their TakeFromGraph callbacks to
+ // be invoked, and ideally they clean up any observers they may have, etc.
+ while (!graph_owned_.empty())
+ auto object = TakeFromGraph(graph_owned_.begin()->first);
+
+ // At this point, all typed observers should be empty.
+ DCHECK(graph_observers_.empty());
+ DCHECK(frame_node_observers_.empty());
+ DCHECK(page_node_observers_.empty());
+ DCHECK(process_node_observers_.empty());
+ DCHECK(system_node_observers_.empty());
+
// All observers should have been removed before the graph is deleted.
// TODO(chrisha): This will disappear, as new observers are allowed to stay
// attached at graph death.
@@ -131,6 +145,26 @@
RemoveObserverImpl(&system_node_observers_, observer);
}
+void GraphImpl::PassToGraph(std::unique_ptr<GraphOwned> graph_owned) {
+ auto* raw = graph_owned.get();
+ DCHECK(!base::Contains(graph_owned_, raw));
+ graph_owned_.insert(std::make_pair(raw, std::move(graph_owned)));
+ raw->OnPassedToGraph(this);
+}
+
+std::unique_ptr<GraphOwned> GraphImpl::TakeFromGraph(GraphOwned* graph_owned) {
+ std::unique_ptr<GraphOwned> object;
+ auto it = graph_owned_.find(graph_owned);
+ if (it != graph_owned_.end()) {
+ DCHECK_EQ(graph_owned, it->first);
+ DCHECK_EQ(graph_owned, it->second.get());
+ object = std::move(it->second);
+ graph_owned_.erase(it);
+ object->OnTakenFromGraph(this);
+ }
+ return object;
+}
+
void GraphImpl::RegisterObserver(GraphImplObserver* observer) {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
observer->SetGraph(this);
diff --git a/chrome/browser/performance_manager/graph/graph_impl.h b/chrome/browser/performance_manager/graph/graph_impl.h
index 780ef83..b7bf449 100644
--- a/chrome/browser/performance_manager/graph/graph_impl.h
+++ b/chrome/browser/performance_manager/graph/graph_impl.h
@@ -13,6 +13,7 @@
#include <utility>
#include <vector>
+#include "base/containers/flat_map.h"
#include "base/macros.h"
#include "base/process/process_handle.h"
#include "base/sequence_checker.h"
@@ -57,6 +58,8 @@
void RemovePageNodeObserver(PageNodeObserver* observer) override;
void RemoveProcessNodeObserver(ProcessNodeObserver* observer) override;
void RemoveSystemNodeObserver(SystemNodeObserver* observer) override;
+ void PassToGraph(std::unique_ptr<GraphOwned> graph_owned) override;
+ std::unique_ptr<GraphOwned> TakeFromGraph(GraphOwned* graph_owned) override;
uintptr_t GetImplType() const override;
const void* GetImpl() const override;
@@ -106,6 +109,9 @@
// Allows explicitly invoking SystemNode destruction for testing.
void ReleaseSystemNodeForTesting() { ReleaseSystemNode(); }
+ // Returns the number of objects in the |graph_owned_| map, for testing.
+ size_t GraphOwnedCountForTesting() const { return graph_owned_.size(); }
+
protected:
friend class NodeBase;
@@ -149,6 +155,10 @@
std::vector<ProcessNodeObserver*> process_node_observers_;
std::vector<SystemNodeObserver*> system_node_observers_;
+ // Graph-owned objects. For now we only expect O(10) clients, hence the
+ // flat_map.
+ base::flat_map<GraphOwned*, std::unique_ptr<GraphOwned>> graph_owned_;
+
// User data storage for the graph.
friend class NodeAttachedDataMapHelper;
using NodeAttachedDataKey = std::pair<const Node*, const void*>;
diff --git a/chrome/browser/performance_manager/graph/graph_impl_unittest.cc b/chrome/browser/performance_manager/graph/graph_impl_unittest.cc
index dc62b1a..3e7d222 100644
--- a/chrome/browser/performance_manager/graph/graph_impl_unittest.cc
+++ b/chrome/browser/performance_manager/graph/graph_impl_unittest.cc
@@ -138,7 +138,7 @@
LenientMockObserver() {}
~LenientMockObserver() override {}
- MOCK_METHOD1(OnBeforeGraphDestroyed, void(const Graph*));
+ MOCK_METHOD1(OnBeforeGraphDestroyed, void(Graph*));
};
using MockObserver = ::testing::StrictMock<LenientMockObserver>;
@@ -150,15 +150,79 @@
TEST(GraphImplTest, ObserverWorks) {
std::unique_ptr<GraphImpl> graph = base::WrapUnique(new GraphImpl());
- const Graph* raw_graph = graph.get();
+ Graph* raw_graph = graph.get();
MockObserver obs;
graph->AddGraphObserver(&obs);
graph->RemoveGraphObserver(&obs);
graph->AddGraphObserver(&obs);
- EXPECT_CALL(obs, OnBeforeGraphDestroyed(raw_graph));
+ // Expect the graph teardown callback to be invoked. We have to unregister our
+ // observer in order to maintain graph invariants.
+ EXPECT_CALL(obs, OnBeforeGraphDestroyed(raw_graph))
+ .WillOnce(testing::Invoke(
+ [&obs](Graph* graph) { graph->RemoveGraphObserver(&obs); }));
graph.reset();
}
+namespace {
+
+class Foo : public GraphOwned {
+ public:
+ explicit Foo(int* destructor_count) : destructor_count_(destructor_count) {}
+
+ ~Foo() override { (*destructor_count_)++; }
+
+ // GraphOwned implementation:
+ void OnPassedToGraph(Graph* graph) override { passed_to_called_ = true; }
+ void OnTakenFromGraph(Graph* graph) override { taken_from_called_ = true; }
+
+ bool passed_to_called() const { return passed_to_called_; }
+ bool taken_from_called() const { return taken_from_called_; }
+
+ private:
+ bool passed_to_called_ = false;
+ bool taken_from_called_ = false;
+ int* destructor_count_ = nullptr;
+};
+
+} // namespace
+
+TEST(GraphImplTest, GraphOwned) {
+ int destructor_count = 0;
+
+ std::unique_ptr<Foo> foo1 = base::WrapUnique(new Foo(&destructor_count));
+ std::unique_ptr<Foo> foo2 = base::WrapUnique(new Foo(&destructor_count));
+ auto* raw1 = foo1.get();
+ auto* raw2 = foo2.get();
+
+ // Pass both objects to the graph.
+ std::unique_ptr<GraphImpl> graph = base::WrapUnique(new GraphImpl());
+ EXPECT_EQ(0u, graph->GraphOwnedCountForTesting());
+ EXPECT_FALSE(raw1->passed_to_called());
+ graph->PassToGraph(std::move(foo1));
+ EXPECT_TRUE(raw1->passed_to_called());
+ EXPECT_EQ(1u, graph->GraphOwnedCountForTesting());
+ EXPECT_FALSE(raw2->passed_to_called());
+ graph->PassToGraph(std::move(foo2));
+ EXPECT_TRUE(raw2->passed_to_called());
+ EXPECT_EQ(2u, graph->GraphOwnedCountForTesting());
+
+ // Take one back.
+ EXPECT_FALSE(raw1->taken_from_called());
+ foo1 = graph->TakeFromGraphAs<Foo>(raw1);
+ EXPECT_TRUE(raw1->taken_from_called());
+ EXPECT_EQ(1u, graph->GraphOwnedCountForTesting());
+
+ // Destroy that object and expect its destructor to have been invoked.
+ EXPECT_EQ(0, destructor_count);
+ foo1.reset();
+ EXPECT_EQ(1, destructor_count);
+
+ // Now destroy the graph and expect the other object to have been torn down
+ // too.
+ graph.reset();
+ EXPECT_EQ(2, destructor_count);
+}
+
} // namespace performance_manager
diff --git a/chrome/browser/performance_manager/public/graph/graph.h b/chrome/browser/performance_manager/public/graph/graph.h
index 5bceb6c0..0cc76500 100644
--- a/chrome/browser/performance_manager/public/graph/graph.h
+++ b/chrome/browser/performance_manager/public/graph/graph.h
@@ -6,12 +6,15 @@
#define CHROME_BROWSER_PERFORMANCE_MANAGER_PUBLIC_GRAPH_GRAPH_H_
#include <cstdint>
+#include <memory>
#include "base/macros.h"
+#include "base/memory/ptr_util.h"
namespace performance_manager {
class GraphObserver;
+class GraphOwned;
class FrameNodeObserver;
class PageNodeObserver;
class ProcessNodeObserver;
@@ -42,6 +45,21 @@
virtual void RemoveProcessNodeObserver(ProcessNodeObserver* observer) = 0;
virtual void RemoveSystemNodeObserver(SystemNodeObserver* observer) = 0;
+ // For convenience, allows you to pass ownership of an object to the graph.
+ // Useful for attaching observers that will live with the graph until it dies.
+ // If you can name the object you can also take it back via "TakeFromGraph".
+ virtual void PassToGraph(std::unique_ptr<GraphOwned> graph_owned) = 0;
+ virtual std::unique_ptr<GraphOwned> TakeFromGraph(
+ GraphOwned* graph_owned) = 0;
+
+ // A TakeFromGraph helper that casts to a derived type. It is up to the caller
+ // to ensure that the cast is safe.
+ template <typename DerivedType>
+ std::unique_ptr<DerivedType> TakeFromGraphAs(GraphOwned* graph_owned) {
+ return base::WrapUnique(
+ static_cast<DerivedType*>(TakeFromGraph(graph_owned).release()));
+ }
+
// The following functions are implementation detail and should not need to be
// used by external clients. They provide the ability to safely downcast to
// the underlying implementation.
@@ -59,18 +77,48 @@
virtual ~GraphObserver();
// Called before the |graph| associated with this observer disappears. This
- // allows the observer to do any necessary cleanup work. Note that the graph
- // is in its destructor while this is being called, so the observer should
- // refrain from uselessly modifying the graph. This is intended to be used to
- // facilitate lifetime management of observers.
- // TODO(chrisha): Make this run before the constructor!
+ // allows the observer to do any necessary cleanup work. Note that the
+ // observer should remove itself from observing the graph using this
+ // callback.
+ // TODO(chrisha): Make this run before the destructor!
// crbug.com/966840
- virtual void OnBeforeGraphDestroyed(const Graph* graph) = 0;
+ virtual void OnBeforeGraphDestroyed(Graph* graph) = 0;
private:
DISALLOW_COPY_AND_ASSIGN(GraphObserver);
};
+// Helper class for passing ownership of objects to a graph.
+class GraphOwned {
+ public:
+ GraphOwned();
+ virtual ~GraphOwned();
+
+ // Called when the object is passed into the graph.
+ virtual void OnPassedToGraph(Graph* graph) = 0;
+
+ // Called when the object is removed from the graph, either via an explicit
+ // call to Graph::TakeFromGraph, or prior to the Graph being destroyed.
+ virtual void OnTakenFromGraph(Graph* graph) = 0;
+
+ private:
+ DISALLOW_COPY_AND_ASSIGN(GraphOwned);
+};
+
+// A default implementation of GraphOwned.
+class GraphOwnedDefaultImpl : public GraphOwned {
+ public:
+ GraphOwnedDefaultImpl();
+ ~GraphOwnedDefaultImpl() override;
+
+ // GraphOwned implementation:
+ void OnPassedToGraph(Graph* graph) override {}
+ void OnTakenFromGraph(Graph* graph) override {}
+
+ private:
+ DISALLOW_COPY_AND_ASSIGN(GraphOwnedDefaultImpl);
+};
+
} // namespace performance_manager
#endif // CHROME_BROWSER_PERFORMANCE_MANAGER_PUBLIC_GRAPH_GRAPH_H_
