Implementation of leveldb-backed PrefStore

base/prefs/leveldb_pref_store.cc

Index: base/prefs/leveldb_pref_store.cc
diff --git a/base/prefs/leveldb_pref_store.cc b/base/prefs/leveldb_pref_store.cc
new file mode 100644
index 0000000000000000000000000000000000000000..da787e449d951128270258fa7004761bbb7bfc09
--- /dev/null
+++ b/base/prefs/leveldb_pref_store.cc
@@ -0,0 +1,167 @@
+// 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.
+
+#include "base/prefs/leveldb_pref_store.h"
+
+#include <algorithm>

[Mattias Nissler (ping if slow), 2014/02/24 09:00:40]

unused?

+#include <set>

[Mattias Nissler (ping if slow), 2014/02/24 09:00:40]

unused?

+
+#include "base/bind.h"
+#include "base/callback.h"
+#include "base/file_util.h"

[Mattias Nissler (ping if slow), 2014/02/24 09:00:40]

unused?

[dgrogan, 2014/02/25 03:32:08]

Needed, but the rest were not.

+#include "base/json/json_file_value_serializer.h"

[Mattias Nissler (ping if slow), 2014/02/24 09:00:40]

unused?

+#include "base/json/json_string_value_serializer.h"
+#include "base/location.h"
+#include "base/memory/ref_counted.h"

[Mattias Nissler (ping if slow), 2014/02/24 09:00:40]

unused?

+#include "base/message_loop/message_loop_proxy.h"

[Mattias Nissler (ping if slow), 2014/02/24 09:00:40]

Unused?

+#include "base/prefs/pref_filter.h"

[Mattias Nissler (ping if slow), 2014/02/24 09:00:40]

unused?

+#include "base/sequenced_task_runner.h"
+#include "base/threading/sequenced_worker_pool.h"

[Mattias Nissler (ping if slow), 2014/02/24 09:00:40]

unused?

+#include "base/values.h"
+#include "third_party/leveldatabase/src/include/leveldb/db.h"

[Mattias Nissler (ping if slow), 2014/02/24 09:00:40]

Also in .h, but that might be able to use a forward-decl.

+
+LevelDBPrefStore::LevelDBPrefStore(
+    const base::FilePath& filename,
+    base::SequencedTaskRunner* sequenced_task_runner)
+    : path_(filename),
+      sequenced_task_runner_(sequenced_task_runner),
+      read_only_(false),

[Mattias Nissler (ping if slow), 2014/02/24 09:00:40]

What's the use case for this?

+      initialized_(false),
+      read_error_(PREF_READ_ERROR_OTHER) {}
+
+bool LevelDBPrefStore::GetValue(const std::string& key,
+                                const base::Value** result) const {
+  const base::Value* tmp = NULL;
+  if (!prefs_.GetValue(key, &tmp)) {
+    return false;
+  }
+
+  if (result)
+    *result = tmp;
+  return true;
+}
+
+// Callers of GetMutableValue have to call ReportValueChanged in order for the
+// new value to be persisted.
+bool LevelDBPrefStore::GetMutableValue(const std::string& key,
+                                       base::Value** result) {
+  return prefs_.GetValue(key, result);
+}
+
+void LevelDBPrefStore::AddObserver(PrefStore::Observer* observer) {
+  observers_.AddObserver(observer);
+}
+
+void LevelDBPrefStore::RemoveObserver(PrefStore::Observer* observer) {
+  observers_.RemoveObserver(observer);
+}
+
+bool LevelDBPrefStore::HasObservers() const {
+  return observers_.might_have_observers();
+}
+
+bool LevelDBPrefStore::IsInitializationComplete() const { return initialized_; }
+
+void LevelDBPrefStore::PersistOnFileThread(const std::string& key,
+                                           const std::string& value) {
+  DCHECK(sequenced_task_runner_->RunsTasksOnCurrentThread());
+
+  leveldb::Status status = db_->Put(leveldb::WriteOptions(), key, value);
+  DCHECK(status.ok());

[Mattias Nissler (ping if slow), 2014/02/24 09:00:40]

Is this OK? What kind of errors can happen here? Even if they're safe to ignore,
please at least put a comment justifying that we're ignoring them here.

[dgrogan, 2014/02/25 03:32:08]

The corresponding error in JSONPrefStore is ignored, so I think it's ok
The two overarching types are Corruption errors and I/O errors.
Sounds good.

+}
+
+void LevelDBPrefStore::PersistFromUIThread(const std::string& key,
+                                           base::Value* value) {
+  if (read_only_)
+    return;
+  std::string value_string;
+  JSONStringValueSerializer serializer(&value_string);
+  DCHECK(serializer.Serialize(*value));

[Mattias Nissler (ping if slow), 2014/02/24 09:00:40]

Please no production code in DCHECK, it won't get executed in release builds :)

[dgrogan, 2014/02/25 03:32:08]

Done.

+
+  sequenced_task_runner_->PostTask(
+      FROM_HERE,
+      base::Bind(
+          &LevelDBPrefStore::PersistOnFileThread, this, key, value_string));

[Mattias Nissler (ping if slow), 2014/02/24 09:00:40]

How do you make sure |this| is still alive when PersistOnFileThead runs on the
other thread?

[dgrogan, 2014/02/25 03:32:08]

If I'm reading
https://code.google.com/p/chromium/codesearch#chromium/src/base/callback.h&l=206
correctly, base::Bind will keep reference counted objects alive correctly. Am I
misunderstanding?

[Mattias Nissler (ping if slow), 2014/02/25 10:51:44]

Note that PrefStore inherits from base::RefCounted, not from
base::RefCountedThreadSafe. Thus, using scoped_refptrs to jump threads is not OK
in this case. FWIW, the reason for PrefStore being base::RefCounted is to be
able to hook them up to multiple different PrefServices, but that all happens on
the UI thread.

So you'll need a different solution here. You might want to think about moving
all the leveldb interactions into an internal object that lives on the
respective thread.

[dgrogan, 2014/02/26 07:10:47]

Wouldn't this just move the problem to the internal object? What would control
the lifetime of the internal object? Do you know of somewhere in the codebase
that follows this pattern that I can model after or look at?

[Mattias Nissler (ping if slow), 2014/02/26 08:35:00]

The difference is that you can destroy that object on the respective thread.
Thus, all operations and deletion are serialized on that thread, which solves
the use-after-free. The LevelDBPrefStore object on the UI thread can still be
the logical owner and post the destruction task on its own destruction. So the
helper object would live on for a little while until tasks on the other thread
complete.

You'll still have the issue of reporting back read results to the
LevelDBPrefStore object, but that can easily be solved with PostTaskAndReply +
WeakPtr (note that WeakPtr may be passed across threads, but dereferencing must
happen on a the same thread the WeakPtr was created).
Code that just needs plain file access often uses one-off PostTaskAndReply
sequences. One complicating factor here is that you need to hold on to the
leveldb::DB* handle created by the open operation (although it might be possible
to just pass the DB pointer back to the UI thread and on to subsequent
operations if you make sure never to actually access the DB object on the UI
thread). Bottom line: I don't know of a good example from the top of my head
that I can see as the ideal fit for your situation. Maybe take a look at what
other consumers of leveldb do?

Another related note: Is the LevelDBPrefStore implementation also supposed to be
used for g_browser_process->local_state()? If so, you're facing the additional
complication that local_state() starts up before the threads are spinning and
goes down after we've cleaned up all threads. Note that
JsonPrefStore/ImportantFileWriter have special handling for the case task
posting fails for exactly this reason.

+}
+
+void LevelDBPrefStore::SetValue(const std::string& key, base::Value* value) {
+  DCHECK(value);
+  scoped_ptr<base::Value> new_value(value);
+  base::Value* old_value = NULL;
+  bool found = prefs_.GetValue(key, &old_value);
+  if (!found || !value->Equals(old_value)) {
+    PersistFromUIThread(key, new_value.get());

[Mattias Nissler (ping if slow), 2014/02/24 09:00:40]

JSONPrefStore batches writes and hits the file only once within 5 second IIRC.
Does leveldb do this internally, or should we batch here as well?

[dgrogan, 2014/02/25 03:32:08]

LevelDB does not do this internally. I thought one of the reasons JSONPrefStore
did that was because the whole file had to be written at once and you didn't
want to write the whole file every time any single pref value changed.
LevelDBPrefStore is able to write key-value pairs separately, avoiding the
write-the-whole-file-at-once problem. So, at least initially, I'd like to avoid
the extra complexity brought on by buffering/batching.

Though, of course, if LevelDBPrefStore is shown to perform worse than
JSONPrefStore, batching here and writing on a timer is one of the first things I
would try.

[Mattias Nissler (ping if slow), 2014/02/25 10:51:44]

OK, are you planning to run performance tests on the two to find out?

[dgrogan, 2014/03/05 03:06:57]

Yes. Note that I don't yet know how, but it's on my list of things to do before
shipping.

+    prefs_.SetValue(key, new_value.release());
+  }
+}
+
+void LevelDBPrefStore::SetValueSilently(const std::string& key,
+                                        base::Value* value) {

[Mattias Nissler (ping if slow), 2014/02/24 09:00:40]

implement?

+}
+
+void LevelDBPrefStore::RemoveValue(const std::string& key) {

[Mattias Nissler (ping if slow), 2014/02/24 09:00:40]

implement?

+}
+
+bool LevelDBPrefStore::ReadOnly() const { return read_only_; }
+
+PersistentPrefStore::PrefReadError LevelDBPrefStore::GetReadError() const {
+  return read_error_;
+}
+
+PersistentPrefStore::PrefReadError LevelDBPrefStore::ReadPrefs() {
+  DCHECK(!db_);
+  if (path_.empty()) {
+    OnStorageRead(PREF_READ_ERROR_FILE_NOT_SPECIFIED, false);
+    return PREF_READ_ERROR_FILE_NOT_SPECIFIED;
+  }
+
+  bool no_dir = !base::PathExists(path_.DirName());
+
+  leveldb::DB* db;
+  leveldb::Options options;
+  options.create_if_missing = true;
+  leveldb::Status status =
+      leveldb::DB::Open(options, path_.AsUTF8Unsafe(), &db);
+  if (!status.ok()) {
+    // TODO(dgrogan): Add error histogram and call RepairDB.
+    OnStorageRead(PREF_READ_ERROR_FILE_OTHER, no_dir);
+    return PREF_READ_ERROR_FILE_OTHER;

[Mattias Nissler (ping if slow), 2014/02/24 09:00:40]

Is it possible to get some more information about the nature of the error and
map that to the PREF_READ_ERROR constants? You may create new ones as well. The
story here is that we've always been seeing pref read errors in practice, and
this enum gives us some insight on what's going on via UMA.

[dgrogan, 2014/03/05 03:06:57]

Done.

+  }
+  db_.reset(db);
+
+  scoped_ptr<leveldb::Iterator> it(db->NewIterator(leveldb::ReadOptions()));
+  for (it->SeekToFirst(); it->Valid(); it->Next()) {
+    const std::string value_string = it->value().ToString();
+    JSONStringValueSerializer deserializer(value_string);
+    std::string error_message;
+    int error_code;
+    base::Value* json_value =
+        deserializer.Deserialize(&error_code, &error_message);
+    DCHECK(json_value) << error_message;

[Mattias Nissler (ping if slow), 2014/02/24 09:00:40]

This should probably not be a DCHECK (unless leveldb catches file corruption?).
If deserialization fails, you will want to skip the SetValue call in the line
below.

[dgrogan, 2014/02/25 03:32:08]

Done.

+    prefs_.SetValue(it->key().ToString(), json_value);
+  }
+
+  PersistentPrefStore::PrefReadError error = PREF_READ_ERROR_NONE;
+  if (!it->status().ok()) {
+    // TODO(dgrogan): Add error histogram?
+    error = PREF_READ_ERROR_FILE_OTHER;
+  }
+  OnStorageRead(error, no_dir);
+  return error;
+}
+
+void LevelDBPrefStore::ReadPrefsAsync(ReadErrorDelegate* error_delegate) {

[Mattias Nissler (ping if slow), 2014/02/24 09:00:40]

implement?

+}
+
+void LevelDBPrefStore::CommitPendingWrite() {

[Mattias Nissler (ping if slow), 2014/02/24 09:00:40]

If this is not needed, please add a comment explaining why.

[dgrogan, 2014/02/25 03:32:08]

Done.

+}
+
+void LevelDBPrefStore::ReportValueChanged(const std::string& key) {

[Mattias Nissler (ping if slow), 2014/02/24 09:00:40]

implement?

+}
+
+void LevelDBPrefStore::OnStorageRead(PersistentPrefStore::PrefReadError error,
+                                     bool no_dir) {

[Mattias Nissler (ping if slow), 2014/02/24 09:00:40]

implement?

+}
+
+LevelDBPrefStore::~LevelDBPrefStore() {}