Make SDCH dictionaries persistent across browser restart.

net/sdch/sdch_owner.cc

Index: net/sdch/sdch_owner.cc
diff --git a/net/sdch/sdch_owner.cc b/net/sdch/sdch_owner.cc
index b0e440569646d0aa8ebd30d2a9ea05297a766b9c..1ceb4d8258030323c87109a78275759082261e45 100644
--- a/net/sdch/sdch_owner.cc
+++ b/net/sdch/sdch_owner.cc
@@ -6,10 +6,15 @@
 
 #include "base/bind.h"
 #include "base/metrics/histogram_macros.h"
+#include "base/prefs/persistent_pref_store.h"
+#include "base/prefs/value_map_pref_store.h"
 #include "base/time/default_clock.h"
+#include "base/values.h"
 #include "net/base/sdch_manager.h"
 #include "net/base/sdch_net_log_params.h"
 
+namespace net {
+
 namespace {
 
 enum DictionaryFate {
@@ -41,6 +46,29 @@ enum DictionaryFate {
   DICTIONARY_FATE_MAX = 9
 };
 
+struct FetchData : public SdchDictionaryFetcher::Data {
+  ~FetchData() override {}
+
+  base::Time last_used;
+  int use_count;
+
+  FetchData() : use_count(0) {}
+  FetchData(const base::Time& last_used, int use_count)
+      : last_used(last_used), use_count(use_count) {}
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(FetchData);
+};
+
+// Dictionaries that haven't been touched in 24 hours may be evicted
+// to make room for new dictionaries.
+const int kHoursFreshSinceLastUsed = 24;
+
+// Newly added dictionaries are consider to have been last touched 23 hours
+// ago, so they aren't kept around long if they're a one-off, but dictionary
+// loading doesn't thrash if many dictionaries are seen in quick succession.
+const int kInitialHoursSinceLastUsed = 23;

[mmenke, 2015/02/02 16:35:28]

May be beyond the scoped of this CL, but this seems like a hack.

I'd suggest just checking "last_used" and replace this with
"kHoursFreshIfNeverUsed", rather than storing bogus last used values. 
Alternatives, could have an expires or eviction time instead of last used.

[Randy Smith (Not in Mondays), 2015/02/04 19:29:03]

No argument, but ...
... I'm not understanding the alternative.  Do you mean leaving last_used null
if it hasn't been used, and handling that explicitly?  If we do that, we lose
the information about when the dictionary was added, which seems important (at
least for the shape of algorithm I currently have).  Either hours fresh if never
used is greater than the stale threshold (in which case we can thrash if there
are two dictionaries that both want to be loaded) or it's less than the stale
threshold (in which case if a dictionary is never used, it'll never be evicted).
 

Doing an expiration time could work.  I'm not thrilled by it--I'd rather keep
data in storage and policy in the code--but I don't have a strong argument
against it.  Let me understand your first alternative, and if that doesn't work,
I"ll do expiration time.

[mmenke, 2015/02/04 19:53:46]

We're already recording the use_count, so I was thinking we store time loaded as
last_used, and have something like:

base::TimeDelta time_since_last_used = Now() - last_used;

if (time_since_last_used < kFreshnessLifetime || (use_count == 0 &&
time_since_last_used < kNeverUsedFreshnessLiftime)) {
  // Evict here.
}

Anyhow, if you prefer what you have over my suggestions, feel free to keep it
as-is, and maybe add a comment about it, or maybe rename time since last used to
something more accurate (If perhaps more painfully verbose. 
time_to_use_for_freshness_calculations?)

+
 void RecordDictionaryFate(enum DictionaryFate fate) {
   UMA_HISTOGRAM_ENUMERATION("Sdch3.DictionaryFate", fate, DICTIONARY_FATE_MAX);
 }
@@ -54,9 +82,68 @@ void RecordDictionaryEviction(int use_count, DictionaryFate fate) {
   RecordDictionaryFate(fate);
 }
 
-}  // namespace
+// Schema specifications and access routines.
+
+// The persistent prefs store is conceptually shared with any other network
+// stack systems that want to persist data over browser restarts, and so
+// use of it must be namespace restricted.
+// Schema:
+//      pref_store_->GetValue(kPreferenceName) -> Dictionary {
+//          'version' -> 1 [int]
+//          'dictionaries' -> Dictionary {
+//              server_hash -> {
+//                  'url' -> URL [string]
+//                  'last_used' -> seconds since unix epoch [double]
+//                  'use_count' -> use count [int]
+//          }
+//      }
+const char kPreferenceName[] = "SDCH";
+const char kVersionName[] = "version";
+const char kDictionariesName[] = "dictionaries";
+const char kDictionaryUrlName[] = "url";
+const char kDictionaryLastUsedName[] = "last_used";
+const char kDictionaryUseCountName[] = "use_count";
+
+const int kVersion = 1;
+
+// This function returns store[kPreferenceName]
+base::DictionaryValue* GetPersistentStoreMap(WriteablePrefStore* store) {
+  base::Value* result = nullptr;
+  bool success = store->GetMutableValue(kPreferenceName, &result);

[Bernhard Bauer, 2015/02/02 14:39:46]

Mutating the value returned by GetMutableValue will *not* trigger the mechanism
that notifies PrefStore observers and automatically commits changes, as we have
no way of knowing when the mutation happens. If you do that, you need to trigger
the notifications manually, by calling ReportValueChanged(). What we usually do
is to wrap accesses to the mutable value in a stack-allocated object that will
do that on destruction (e.g. ScopedUserPrefUpdate).

[Randy Smith (Not in Mondays), 2015/02/04 19:29:03]

Ah!  Thank you for the heads up.  I can't use ScopedUserPrefUpdate (it uses
PrefService and requires the UI thread), and I decided that this wasn't worth my
writing a wrapper class for base::DictionaryValue* that would combine the
functionality of DictionaryValue and ScopedUserPrefUpdate, so I just wrote a
simple ScopedPrefNotifier class and paired it appropriately with calls to
getting mutable values from the preference store.  Let me know what you think.

[Bernhard Bauer, 2015/02/04 21:48:00]

Yes, that's what I meant actually (I just mentioned ScopedUserPrefUpdate as an
example of the pattern). I should have been clearer.

+  DCHECK(success);
+
+  base::DictionaryValue* dictionary_result = nullptr;
+  success = result->GetAsDictionary(&dictionary_result);
+  DCHECK(success);
+  DCHECK(dictionary_result);
+
+  return dictionary_result;
+}
 
-namespace net {
+// This function returns store[kPreferenceName/kDictionaries]
+base::DictionaryValue* GetPersistentStoreDictionaryMap(
+    WriteablePrefStore* store) {
+  base::DictionaryValue* dictionary_result = nullptr;
+  bool success = GetPersistentStoreMap(store)
+                     ->GetDictionary(kDictionariesName, &dictionary_result);
+  DCHECK(success);
+  DCHECK(dictionary_result);
+
+  return dictionary_result;
+}
+
+// This function initializes a pref store with an empty version of the
+// above schema, removing anything previously in the store under
+// kPreferenceName.
+void InitializePersistentStore(WriteablePrefStore* store) {
+  base::DictionaryValue* empty_store(new base::DictionaryValue);
+  empty_store->SetInteger(kVersionName, kVersion);
+  empty_store->Set(kDictionariesName,
+                   make_scoped_ptr(new base::DictionaryValue));
+  store->SetValue(kPreferenceName, empty_store);
+}
+
+}  // namespace
 
 // Adjust SDCH limits downwards for mobile.
 #if defined(OS_ANDROID) || defined(OS_IOS)
@@ -86,6 +173,9 @@ SdchOwner::SdchOwner(net::SdchManager* sdch_manager,
       clock_(new base::DefaultClock),
       max_total_dictionary_size_(kMaxTotalDictionarySize),
       min_space_for_dictionary_fetch_(kMinSpaceForDictionaryFetch),
+      in_memory_pref_store_(new ValueMapPrefStore()),
+      persistent_pref_store_(nullptr),
+      pref_store_(in_memory_pref_store_.get()),
       memory_pressure_listener_(
           base::Bind(&SdchOwner::OnMemoryPressure,
                      // Because |memory_pressure_listener_| is owned by
@@ -93,6 +183,7 @@ SdchOwner::SdchOwner(net::SdchManager* sdch_manager,
                      // for the lifetime of |memory_pressure_listener_|.
                      base::Unretained(this))) {
   manager_->AddObserver(this);
+  InitializePersistentStore(pref_store_);
 }
 
 SdchOwner::~SdchOwner() {
@@ -102,6 +193,17 @@ SdchOwner::~SdchOwner() {
                              DICTIONARY_FATE_EVICT_FOR_DESTRUCTION);
   }
   manager_->RemoveObserver(this);
+  if (pref_store_)
+    pref_store_->RemoveObserver(this);
+}
+
+void SdchOwner::EnablePersistentStorage(PersistentPrefStore* pref_store) {
+  DCHECK(!persistent_pref_store_);

[mmenke, 2015/02/02 16:35:28]

include base/logging.h

[Randy Smith (Not in Mondays), 2015/02/04 19:29:03]

Done.

+  persistent_pref_store_ = pref_store;
+  persistent_pref_store_->AddObserver(this);
+
+  if (persistent_pref_store_->IsInitializationComplete())
+    OnInitializationCompleted(true);
 }
 
 void SdchOwner::SetMaxTotalDictionarySize(size_t max_total_dictionary_size) {
@@ -113,9 +215,11 @@ void SdchOwner::SetMinSpaceForDictionaryFetch(
   min_space_for_dictionary_fetch_ = min_space_for_dictionary_fetch;
 }
 
-void SdchOwner::OnDictionaryFetched(const std::string& dictionary_text,
-                                    const GURL& dictionary_url,
-                                    const net::BoundNetLog& net_log) {
+void SdchOwner::OnDictionaryFetched(
+    const std::string& dictionary_text,
+    const GURL& dictionary_url,
+    scoped_ptr<SdchDictionaryFetcher::Data> extra_data,
+    const net::BoundNetLog& net_log) {
   struct DictionaryItem {
     base::Time last_used;
     std::string server_hash;
@@ -138,9 +242,13 @@ void SdchOwner::OnDictionaryFetched(const std::string& dictionary_text,
     }
   };
 
+  // Figure out if there is space for the incoming dictionary; evict
+  // stale dictionaries if needed to make space.
+
   std::vector<DictionaryItem> stale_dictionary_list;
   size_t recoverable_bytes = 0;
-  base::Time stale_boundary(clock_->Now() - base::TimeDelta::FromDays(1));
+  base::Time stale_boundary(
+      clock_->Now() - base::TimeDelta::FromHours(kHoursFreshSinceLastUsed));
   for (auto used_it = local_dictionary_info_.begin();
        used_it != local_dictionary_info_.end(); ++used_it) {
     if (used_it->second.last_used < stale_boundary) {
@@ -161,21 +269,32 @@ void SdchOwner::OnDictionaryFetched(const std::string& dictionary_text,
     return;
   }
 
-  // Evict from oldest to youngest until we have space.
+  base::DictionaryValue* pref_dictionary_map =
+      GetPersistentStoreDictionaryMap(pref_store_);
+
   std::sort(stale_dictionary_list.begin(), stale_dictionary_list.end());
   size_t avail_bytes = max_total_dictionary_size_ - total_dictionary_bytes_;
   auto stale_it = stale_dictionary_list.begin();
   while (avail_bytes < dictionary_text.size() &&
          stale_it != stale_dictionary_list.end()) {
     manager_->RemoveSdchDictionary(stale_it->server_hash);
-    RecordDictionaryEviction(stale_it->use_count,
-                             DICTIONARY_FATE_EVICT_FOR_DICT);
     local_dictionary_info_.erase(stale_it->server_hash);
+
+    DCHECK(pref_dictionary_map->HasKey(stale_it->server_hash));
+    bool success = pref_dictionary_map->RemoveWithoutPathExpansion(
+        stale_it->server_hash, nullptr);
+    DCHECK(success);
+
     avail_bytes += stale_it->dictionary_size;
+
+    RecordDictionaryEviction(stale_it->use_count,
+                             DICTIONARY_FATE_EVICT_FOR_DICT);
+
     ++stale_it;
   }
   DCHECK(avail_bytes >= dictionary_text.size());
 
+  // Add the new dictionary.
   std::string server_hash;
   net::SdchProblemCode rv = manager_->AddSdchDictionary(
       dictionary_text, dictionary_url, &server_hash);
@@ -190,14 +309,38 @@ void SdchOwner::OnDictionaryFetched(const std::string& dictionary_text,
 
   RecordDictionaryFate(DICTIONARY_FATE_ADDED);
 
+  total_dictionary_bytes_ += dictionary_text.size();
+
+  DictionaryInfo dictionary_info;
+  dictionary_info.size = dictionary_text.size();
+  if (extra_data) {
+    // This dictionary was persisted from a previous instance of the
+    // browser; copy the usage information across from that instance.
+    FetchData* sdch_fetch_data = static_cast<FetchData*>(extra_data.get());

[Bernhard Bauer, 2015/02/02 14:39:46]

An alternative to passing this as SdchDictionaryFetcher::Data and then having to
cast it back to FetchData would be to bind the data in a callback instead. That
means that instead of initializing the |fetcher_| with the
SdchOwner::OnDictionaryFetched() callback and passing the data directly to
ScheduleReload(), you would pass the callback already bound to the data.

[mmenke, 2015/02/02 16:35:28]

+1 to binding it...Actually, could make the prototype:

SdchOwner::OnDictionaryFetched(
    base::Time last_used,
    int use_count,
    const GURL& dictionary_url,
    const std::string& dictionary_text,
    const net::BoundNetLog& net_log);

And always bind the first two values.

[Randy Smith (Not in Mondays), 2015/02/04 19:29:03]

Good point; that's an excellent idea that cleans up some extra code.  Thank you.
 Done.

[Randy Smith (Not in Mondays), 2015/02/04 19:29:03]

Yep.  This makes several things much simpler.  Thank you.  Done.

+    dictionary_info.last_used = sdch_fetch_data->last_used;
+    dictionary_info.use_count = sdch_fetch_data->use_count;
+  } else {
+    // This is a newly added dictionary; the use count will be zero, and
+    // the last used time set to something that will avoid thrashing, but
+    // won't keep the dictionary in memory for too long if it's never used.
+    dictionary_info.last_used =
+        clock_->Now() - base::TimeDelta::FromHours(kInitialHoursSinceLastUsed);
+    dictionary_info.use_count = 0;
+  }
+
   DCHECK(local_dictionary_info_.end() ==
          local_dictionary_info_.find(server_hash));
-  total_dictionary_bytes_ += dictionary_text.size();
-  local_dictionary_info_[server_hash] = DictionaryInfo(
-      // Set the time last used to something to avoid thrashing, but not recent,
-      // to avoid taking too much time/space with useless dictionaries/one-off
-      // visits to web sites.
-      clock_->Now() - base::TimeDelta::FromHours(23), dictionary_text.size());
+  local_dictionary_info_[server_hash] = dictionary_info;
+
+  // Record the addition in the pref store.
+  scoped_ptr<base::DictionaryValue> dictionary_description(
+      new base::DictionaryValue());
+  dictionary_description->SetString(kDictionaryUrlName, dictionary_url.spec());
+  dictionary_description->SetDouble(kDictionaryLastUsedName,
+                                    dictionary_info.last_used.ToDoubleT());
+  dictionary_description->SetInteger(kDictionaryUseCountName,
+                                     dictionary_info.use_count);
+  pref_dictionary_map->Set(server_hash, dictionary_description.Pass());
 }
 
 void SdchOwner::OnDictionaryUsed(SdchManager* manager,
@@ -207,6 +350,14 @@ void SdchOwner::OnDictionaryUsed(SdchManager* manager,
 
   it->second.last_used = clock_->Now();
   it->second.use_count++;

[mmenke, 2015/02/02 16:35:28]

optional:  I'm not a big fan of storing things twice.  May want to just get rid
of local_dictionary_info_ in favor of always using the pref dictionary. 
Admittedly, it's heavier weight, and doesn't save much memory - but my concern
is the more copies of data we have, the more chance there is for it to end up
inconsistent, due to a bug.

Of course, with the pref, there's always the question "What if the dictionary
was removed from the pref store".  Anyhow, up to you.

[Randy Smith (Not in Mondays), 2015/02/04 19:29:03]

Yeah, I had the same thought--just hadn't gotten to it yet.  Done.

+
+  base::DictionaryValue* pref_dictionary_map =
+      GetPersistentStoreDictionaryMap(pref_store_);
+
+  pref_dictionary_map->SetInteger(server_hash + "/use_count",
+                                  it->second.use_count);
+  pref_dictionary_map->SetDouble(server_hash + "/last_used",
+                                 it->second.last_used.ToDoubleT());

[mmenke, 2015/02/02 16:35:28]

Shouldn't you be updating the dictionary fields you added at the bottom of the
above function?

[Randy Smith (Not in Mondays), 2015/02/04 19:29:03]

I presume you mean "shouldn't you be using the constants for those fields that
you've carefully setup rather than magic strings"?  Yep, I should.  Done.

 }
 
 void SdchOwner::OnGetDictionary(net::SdchManager* manager,
@@ -234,13 +385,53 @@ void SdchOwner::OnGetDictionary(net::SdchManager* manager,
     return;
   }
 
-  fetcher_.Schedule(dictionary_url);
+  fetcher_.Schedule(dictionary_url,
+                    scoped_ptr<SdchDictionaryFetcher::Data>().Pass());
 }
 
 void SdchOwner::OnClearDictionaries(net::SdchManager* manager) {
   total_dictionary_bytes_ = 0;
   local_dictionary_info_.clear();
   fetcher_.Cancel();
+
+  GetPersistentStoreMap(pref_store_)
+      ->Set(kDictionariesName, new base::DictionaryValue());
+}
+
+void SdchOwner::OnPrefValueChanged(const std::string& key) {
+}
+
+void SdchOwner::OnInitializationCompleted(bool succeeded) {
+  // Error handling is simple: Don't read in any dictionaries and let
+  // normal user behavior fault them back in as needed.
+  // TODO(rdsmith): Decide whether to actually flush things to the
+  // persistent store in this case.
+  if (!succeeded)
+    return;
+
+  // Load in what was stored before chrome exited previously.
+  const base::Value* sdch_persistence_value = nullptr;
+  const base::DictionaryValue* sdch_persistence_dictionary = nullptr;
+
+  // The GetPersistentStore() routine above assumes data formatted
+  // according to the schema described at the top of this file.  Since
+  // this data comes from disk, to avoid disk corruption resulting in
+  // persistent chrome errors this code avoids those assupmtions.
+  if (persistent_pref_store_->GetValue(kPreferenceName,
+                                       &sdch_persistence_value) &&
+      sdch_persistence_value->GetAsDictionary(&sdch_persistence_dictionary)) {
+    LoadPersistedDictionaries(*sdch_persistence_dictionary);
+  }
+
+  // Reset the persistent store and update it with the accumulated
+  // information from the local store.
+  InitializePersistentStore(persistent_pref_store_);
+  GetPersistentStoreDictionaryMap(persistent_pref_store_)
+      ->Swap(GetPersistentStoreDictionaryMap(in_memory_pref_store_.get()));
+
+  // Switch the class pointer and free the old store.
+  pref_store_ = persistent_pref_store_;
+  in_memory_pref_store_ = nullptr;
 }
 
 void SdchOwner::SetClockForTesting(scoped_ptr<base::Clock> clock) {
@@ -262,4 +453,47 @@ void SdchOwner::OnMemoryPressure(
   manager_->ClearData();
 }
 
+bool SdchOwner::LoadPersistedDictionaries(
+    const base::DictionaryValue& persisted_info) {
+  // Any schema error will result in dropping the persisted info.
+  int version = 0;
+  if (!persisted_info.GetInteger(kVersionName, &version))
+    return false;
+
+  // TODO(rdsmith): Handle version upgrades.

[mmenke, 2015/02/02 16:35:28]

Note:  I'm not sure we care enough to do this.  Just losing the old index is not
the end of the world, and the added complexity of additions tests, and
remembering to remove the upgrade code at some point, doesn't seem worth the
effort to me.

[Randy Smith (Not in Mondays), 2015/02/04 19:29:03]

Good point.  I wasn't planning to do any work (besides writing the TODO :-}) in
this CL, but you're right, I'll put a stake in the ground saying we throw away
data on version upgrades.

+  if (version != 1)
+    return false;
+
+  const base::DictionaryValue* dictionary_set = nullptr;
+  if (!persisted_info.GetDictionary(kDictionariesName, &dictionary_set))
+    return false;
+
+  for (base::DictionaryValue::Iterator dict_it(*dictionary_set);
+       !dict_it.IsAtEnd(); dict_it.Advance()) {
+    const base::DictionaryValue* dict_info = nullptr;
+    if (!dict_it.value().GetAsDictionary(&dict_info))
+      return false;

[mmenke, 2015/02/02 16:35:28]

Should we return early here?  Currently, we schedule reloads of all dictionaries
before the first malformed entry in the dictionary, and then ignore the rest,
even if they're also well-formed.

I think we should be consistent, unless there's some reason to believe that all
entries after the first broken one will also be broken in ways we may not be
able to notice.  Either load all well-formed entries, regardless of the presence
of a malformed one, or load non in the presence of a malformed entry.

[Randy Smith (Not in Mondays), 2015/02/04 19:29:03]

Good point.  Done.

+
+    std::string url_string;
+    if (!dict_info->GetString(kDictionaryUrlName, &url_string))
+      return false;
+    GURL dict_url(url_string);
+
+    double last_used = 0.0;

[mmenke, 2015/02/02 16:35:28]

No need to initialize this or use_count.

[Randy Smith (Not in Mondays), 2015/02/04 19:29:03]

Hmmm.  That seems like it's relying on the interface contract of DictionaryValue
(or on ASAN & etc. to find the uninitialized memory read), which is information
that's pretty distant from this location in the code, so it makes me a little
uncomfortable.  I'll change it, but it makes me a bit uncomfortable (though the
code breaks if that interface contract isn't followed, and if we don't
initialize, ASAN & etc. might catch that breakage, so you could make an argument
in favor of not initializing.  Still, it feels weird :-}.)

+    if (!dict_info->GetDouble(kDictionaryLastUsedName, &last_used))
+      return false;
+
+    int use_count = 0;
+    if (!dict_info->GetInteger(kDictionaryUseCountName, &use_count))
+      return false;
+
+    scoped_ptr<FetchData> data(
+        new FetchData(base::Time::FromDoubleT(last_used), use_count));
+
+    fetcher_.ScheduleReload(dict_url, data.Pass());
+  }
+
+  return true;
+}
+
 }  // namespace net