Performance Monitor Database

chrome/browser/performance_monitor/database.h

Index: chrome/browser/performance_monitor/database.h
diff --git a/chrome/browser/performance_monitor/database.h b/chrome/browser/performance_monitor/database.h
new file mode 100644
index 0000000000000000000000000000000000000000..2bf31b18f22ae5791ead60e9df5d7e5fe115e842
--- /dev/null
+++ b/chrome/browser/performance_monitor/database.h
@@ -0,0 +1,129 @@
+// Copyright (c) 2012 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef CHROME_BROWSER_PERFORMANCE_MONITOR_DATABASE_H_
+#define CHROME_BROWSER_PERFORMANCE_MONITOR_DATABASE_H_
+#pragma once
+
+#include <vector>
+#include <map>
+#include <string>
+
+#include "base/file_path.h"
+#include "base/gtest_prod_util.h"
+#include "base/memory/ref_counted.h"
+#include "base/memory/scoped_ptr.h"
+#include "base/memory/scoped_vector.h"
+#include "base/time.h"
+#include "third_party/leveldatabase/src/include/leveldb/db.h"
+
+namespace performance_monitor {
+
+// The class that the database will use to infer time. Abstracting out the time
+// mechanism allows for easy testing and mock data insetion.
+class DBClock : public base::RefCountedThreadSafe<DBClock> {

[Aaron Boodman, 2012/06/08 19:33:56]

These should be nested classes of Database and lose the 'db' prefix.

[Aaron Boodman, 2012/06/08 19:33:56]

Why is it necessary for such a simple class to be refcounted?

[eaugusti, 2012/06/08 22:52:08]

We had it like this so that tests and the db could share the same clock and the
tests could modify the clock as the db was running. But, I guess just using a
linked_ptr would probably be a better choice.

+ public:
+  DBClock() {}
+  virtual base::Time GetTime() = 0;
+ protected:
+  friend class base::RefCountedThreadSafe<DBClock>;
+  virtual ~DBClock() {}
+};
+
+// By default, the database uses a clock that simply returns the current time.
+class SystemClock : public DBClock {
+ public:
+  SystemClock() {}
+  virtual base::Time GetTime() OVERRIDE;
+ private:
+  virtual ~SystemClock() {}
+};
+
+// The class supporting all performance monitor storage. This class wraps
+// multiple leveldb::DB objects. Most methods must be called on a background
+// thread and are named to reflect that. Callers should use
+// BrowserThread::PostBlockingPoolSequencedTask using
+// performance_monitor::kDBSequenceToken as the sequence token.
+class Database : public base::RefCountedThreadSafe<Database> {
+ public:
+  typedef std::vector<std::pair<base::Time, base::Time> > TimePairs;

[Aaron Boodman, 2012/06/08 19:33:56]

std::pair is almost always a mistake. "first" and "second" are meaningless when
reading code. I haven't seen how this is used yet, but I already suggest a
struct with named fields :)

[eaugusti, 2012/06/08 22:52:08]

Done.

+
+  // A "state" value is anything that can only have one value at a time, and
+  // usually describes the state of the browser eg. version.
+  bool AddStateValueOnBackgroundThread(const std::string& key,
+                                       const std::string& value);
+
+  std::string GetStateValueOnBackgroundThread(const std::string& key);
+
+  // Erase everything in the database. Warning - No undo!
+  void ClearOnBackgroundThread();
+
+  // Returns a list of pairs of (start time, end time).
+  TimePairs GetUptimeOnBackgroundThread(const base::Time& start,
+                                        const base::Time& end);
+
+  FilePath path() const { return path_; }
+
+  void set_clock(scoped_refptr<DBClock> clock) {
+    clock_ = clock;
+  }
+
+ private:
+  friend class base::RefCountedThreadSafe<Database>;
+  FRIEND_TEST_ALL_PREFIXES(PerformanceMonitorDatabaseSetupTest,

[Aaron Boodman, 2012/06/08 19:33:56]

friends are often a smell. Can the functionality the tests rely upon be made
generally public?

[eaugusti, 2012/06/08 22:52:08]

We were going to have only the performance monitor main class be able to
construct a Database.

+                           OpenCloseDefaultTest);
+  FRIEND_TEST_ALL_PREFIXES(PerformanceMonitorDatabaseSetupTest,
+                           OpenCloseAlternatePathTest);
+  FRIEND_TEST_ALL_PREFIXES(PerformanceMonitorDatabaseSetupTest, UptimeTest);
+
+  typedef std::map<std::string, leveldb::DB*> LevelDbMap;
+
+  Database(const FilePath& path, scoped_refptr<DBClock> clock);
+  virtual ~Database();
+
+  static scoped_refptr<Database> InitOnBackgroundThread(

[Aaron Boodman, 2012/06/08 21:40:05]

This class seems to have no public way to get an instance. How are people going
to call its public instance methods?

[eaugusti, 2012/06/08 22:52:08]

Through the main performance monitor class.

+      FilePath path, scoped_refptr<DBClock> clock);
+
+  static scoped_refptr<Database> InitOnBackgroundThread(FilePath path) {
+    return InitOnBackgroundThread(path, new SystemClock());
+  }
+
+  static scoped_refptr<Database> InitOnBackgroundThread() {
+    return InitOnBackgroundThread(FilePath(), new SystemClock());
+  }
+
+  bool CloseOnBackgroundThread();
+
+  // Get the leveldb::DB object refered to by |id|.
+  // If one does not yet exist, it will be created.
+  leveldb::DB* FetchDBOnBackgroundThread(const std::string& id);
+
+  // Mark the database as bring active for the current time.

[Aaron Boodman, 2012/06/08 21:40:05]

s/bring/being/?

[eaugusti, 2012/06/08 22:52:08]

Done.

+  void UpdateUptimeOnBackgroundThread();
+
+  // The directory where all the databases will reside.
+  FilePath path_;
+
+  // The key for the beginning of the uptime.
+  std::string start_time_key_;
+
+  // The last time the database had a transaction.
+  base::Time last_update_time_;
+
+  // The amount of time that the db has to be inactive to be considered "down".
+  base::TimeDelta uptime_threshold_;
+
+  scoped_refptr<DBClock> clock_;
+
+  // Mapping of names to LevelDB objects.
+  LevelDbMap databases_;

[Aaron Boodman, 2012/06/08 19:33:56]

It appears that there are a fixed set of known leveldb::DB instances. Instead of
having a map of these, can they just be explicit members of this class?

[Aaron Boodman, 2012/06/08 21:40:05]

Also, I think that it would be good to briefly describe the schema for each of
these databases, and why they were chosen the way they were.

For example, the uptime database is not immediately intuitive. It would be good
to describe the usage so the reader doesn't have to piece it together from the
code.

[eaugusti, 2012/06/08 22:52:08]

Done.

+
+  leveldb::ReadOptions read_options_;
+  leveldb::WriteOptions write_options_;
+
+  DISALLOW_COPY_AND_ASSIGN(Database);
+};
+}  // namespace performance_monitor
+
+#endif  // CHROME_BROWSER_PERFORMANCE_MONITOR_DATABASE_H_