leveldb: Add DBTracker for exposing databases to Chrome's memory-infra.

third_party/leveldatabase/env_chromium.h

Index: third_party/leveldatabase/env_chromium.h
diff --git a/third_party/leveldatabase/env_chromium.h b/third_party/leveldatabase/env_chromium.h
index d4335810b24496abaf1835a01ada393d9bdbbc43..7f063d055dbc21aba95d1817f9409b8f2af26a89 100644
--- a/third_party/leveldatabase/env_chromium.h
+++ b/third_party/leveldatabase/env_chromium.h
@@ -6,13 +6,18 @@
 #define THIRD_PARTY_LEVELDATABASE_ENV_CHROMIUM_H_
 
 #include <deque>
+#include <functional>
+#include <memory>
 #include <set>
 #include <string>
 #include <vector>
 
+#include "base/containers/linked_list.h"
 #include "base/files/file.h"
 #include "base/files/file_path.h"
+#include "base/macros.h"
 #include "base/metrics/histogram.h"
+#include "leveldb/db.h"
 #include "leveldb/env.h"
 #include "port/port_chromium.h"
 #include "util/mutexlock.h"
@@ -230,6 +235,50 @@ class ChromiumEnv : public leveldb::Env,
   LockTable locks_;
 };
 
+// Database registry. Keeps track of all live databases and reports them to
+// memory-infra. This class is thread safe.
+class DBRegistry {

[pwnall, 2017/06/22 02:50:22]

I'd prefer that Open() and VisitOpenDatabases() become functions in the
leveldb_env namespace, so calls look like leveldb_env::Open().
leveldb_env::VisitOpenDatabases can proxy to the DBRegistry singleton instance,
which becomes an implementation detail.

I prefer the name DBTracker to DBRegistry, because I think it conveys the
purpose better. Also, I'd prefer that Register / Unregister become
DatabaseOpened() / DatabaseClosed(), as this clarifies when they're supposed to
be called.

[DmitrySkiba, 2017/06/26 20:45:18]

Renaming: done.

I'm not sure about free-standing functions. In my view the class serves as a
clustering point for all related functionality. I.e. if you see that
DBTracker::Open() is called, you know that there are probably more stuff in
DBTracker.

leveldb_env::Open() doesn't tell much - what is opened, and why not DB::Open?
DBTracker::Open() provides more context.

Besides, the class provides convenient and unambiguous place to add new
functionality. For example along with your comments I've added
GetMemoryDumpName() API so that other MDPs could either provide additional
per-database info, or reference database dumps.

[pwnall, 2017/06/26 23:37:13]

This comment (that I agree with) suggests that leveldb_env::OpenDatabase() /
OpenDB() would be better names than a naked Open(). I think that the context
given by DBTracker is unnecessary mental overhead for most readers, who just
want to get a leveldb::DB instance and get on with their lives.
I'm not opposed to having the class for tracking-related functionality. Open /
OpenDatabase / OpenDB should definitely be exposed in the leveldb namespace, as
the tracking should be an implementation detail. VisitOpenDatabases can remain
in the tracker class, if that makes more sense.

Also, in the interest of keeping CLs manageable in terms of size and review
time, please avoid adding more stuff after the review has started. Dependent CLs
FTW :)

[DmitrySkiba, 2017/06/28 20:35:28]

Done.

+ public:
+  // Opens a database. Internally calls leveldb::DB::Open(options, name)

[pwnall, 2017/06/22 02:50:22]

Can you please write the docs from the perspective of a class user?

In this case, I think it makes more sense to state that the returned instances
are tracked by the registry so they're available in VisitDatabases. I don't
think the class user cares how the wrapping works, so there's no reason to
commit to specifics in the docs.

[DmitrySkiba, 2017/06/26 20:45:19]

Done.

+  // and wraps the result in |DBWrapper| instance (see below).
+  static leveldb::Status Open(const leveldb::Options& options,
+                              const std::string& name,
+                              leveldb::DB** dbptr);
+
+  static DBRegistry* GetInstance();
+
+  // DB wrapper that provides extra information about the database.
+  class DBWrapper : public leveldb::DB {
+   public:
+    // DB name that Open() was called with.
+    virtual const std::string& name() const = 0;

[pwnall, 2017/06/22 02:50:22]

Do you envision adding more functionality here, such as iterating over a
database's open iterators?

If not, it might be simpler to add the name parameter to the DatabaseVisitor
function and remove this interface altogether.

[DmitrySkiba, 2017/06/26 20:45:19]

Yes, more stuff is planned: visiting iterators, exposing caches (similarly to
https://chromium-review.googlesource.com/543439).

[pwnall, 2017/06/26 23:37:13]

How about TrackedDB instead of DB for the name, then? I'm not a big fan of empty
words like wrapper / proxy / decorator in APIs. The class comment can then state
that the class exposes the information tracked by DBTracker.

[DmitrySkiba, 2017/06/28 20:35:28]

Done.

+  };
+
+  using DatabaseVisitor = std::function<void(DBWrapper*)>;

[pwnall, 2017/06/22 02:50:22]

Visitor?

[DmitrySkiba, 2017/06/26 20:45:18]

I think that if method is called VisitDatabases(), then 'DatabaseVisitor' makes
more sense.

[pwnall, 2017/06/26 23:37:13]

I thought Visitor is acceptable if this is the only visitor, but I suppose we'll
have an iterator visitor as well at some point in the near future.

[DmitrySkiba, 2017/06/28 20:35:28]

Acknowledged.

+
+  // Calls |visitor| for each live database. Takes a lock, preventing
+  // any database from being created or destroyed (but doesn't lock the
+  // databases themselves).
+  void VisitDatabases(const DatabaseVisitor& visitor);

[pwnall, 2017/06/22 02:50:22]

VisitOpenDatabases?

[DmitrySkiba, 2017/06/26 20:45:19]

I feel that 'Open' is redundant, since it's the only option (i.e. we're not
supporting visiting 'Closed' databases).

[pwnall, 2017/06/26 23:37:13]

It is, once you've thought things through. However, having it in is useful for a
reader that isn't fully familiar with the leveldb::DB lifecycle. For example, if
I forget how leveldb works, I'll wonder if it's possible to have DB instances
that have been created but not "opened" yet, and instances that have been closed
but not released, and I'll wonder what gets visited and what doesn't.

I don't want to force readers to remember how LevelDB works to be able to reason
through this code.

[DmitrySkiba, 2017/06/28 20:35:28]

I think this is job for comments, so I updated the comment for VisitDatabases().
Now it mentions that 'live' databases are visited and gives exact criteria for
'liveness'.

I think this makes things easier to understand - you open a database, and it's
enumerated until its instance is destroyed. That would be the case even if there
was Close() method in leveldb::DB. And BTW I was surprised to find out that in
leveldb world 'closing a database' == 'destroying its instance' (I found out
about that when I was writing tests) - i.e. I designed the API before that,
without that specific leveldb knowledge.

So the summary is that I don't think 'Open' makes sense there exactly because
it's natural to expect something to be enumerated until it's dead, and the fact
that in leveldb world 'closed' == 'dead' is an uncommon implementation detail.

+
+ private:
+  class DBWrapperImpl;
+  class MDP;

[pwnall, 2017/06/22 02:50:22]

Can you please use the full name MemoryDumpProvider? I don't think this is an
obvious abbreviation for people outside memory infra.

[DmitrySkiba, 2017/06/26 20:45:19]

Done.

+
+  DBRegistry();
+  ~DBRegistry();
+
+  void RegisterDatabase(DBWrapperImpl* database);
+  void UnregisterDatabase(DBWrapperImpl* database);
+
+  std::unique_ptr<MDP> mdp_;
+
+  base::Lock databases_lock_;
+  base::LinkedList<DBWrapperImpl> databases_;
+
+  DISALLOW_COPY_AND_ASSIGN(DBRegistry);
+};
+
 }  // namespace leveldb_env
 
 #endif  // THIRD_PARTY_LEVELDATABASE_ENV_CHROMIUM_H_