SDCH: add TimeWeightedMemoryUse

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 9d985816a9e0350e3930b7c17e7f4f0fdbb85630..9f0922ceaee19dd62ae8c0ec0675fc39e7190e06 100644
--- a/net/sdch/sdch_owner.cc
+++ b/net/sdch/sdch_owner.cc
@@ -10,6 +10,7 @@
 #include "base/metrics/histogram_macros.h"
 #include "base/prefs/persistent_pref_store.h"
 #include "base/prefs/value_map_pref_store.h"
+#include "base/process/process_info.h"
 #include "base/strings/string_util.h"
 #include "base/time/default_clock.h"
 #include "base/values.h"
@@ -177,6 +178,7 @@ class DictionaryPreferenceIterator {
   base::Time last_used() const { return last_used_; }
   int use_count() const { return use_count_; }
   int size() const { return size_; }
+  base::Time load_time() const { return load_time_; }
 
  private:
   void LoadDictionaryOrDie();
@@ -186,6 +188,7 @@ class DictionaryPreferenceIterator {
   base::Time last_used_;
   int use_count_;
   int size_;
+  base::Time load_time_;

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

I'm probably missing something really obvious, but why this member variable?  I
don't see it ever being set, and my memory of the design you settled on was that
load_time_ wouldn't be stored in the persistent store.

(Doing the rest of the review assuming that this is a leftover bit of cruft from
an earlier implementation.)

[Elly Fong-Jones, 2015/04/03 15:21:00]

Yeah, this was vestigal. Removed.

 
   base::DictionaryValue::Iterator dictionary_iterator_;
 };
@@ -310,6 +313,7 @@ SdchOwner::~SdchOwner() {
                           DICTIONARY_FATE_UNLOAD_FOR_DESTRUCTION :
                           DICTIONARY_FATE_EVICT_FOR_DESTRUCTION;
     RecordDictionaryEvictionOrUnload(new_uses, fate);
+    RecordDictionaryLifetime(it.server_hash(), it.size());
   }
   manager_->RemoveObserver(this);
 
@@ -351,16 +355,19 @@ void SdchOwner::OnDictionaryFetched(base::Time last_used,
     std::string server_hash;
     int use_count;
     size_t dictionary_size;
+    base::Time load_time;
 
     DictionaryItem() : use_count(0), dictionary_size(0) {}
     DictionaryItem(const base::Time& last_used,
                    const std::string& server_hash,
                    int use_count,
-                   size_t dictionary_size)
+                   size_t dictionary_size,
+                   const base::Time& load_time)
         : last_used(last_used),
           server_hash(server_hash),
           use_count(use_count),
-          dictionary_size(dictionary_size) {}
+          dictionary_size(dictionary_size),
+          load_time(load_time) {}
     DictionaryItem(const DictionaryItem& rhs) = default;
     DictionaryItem& operator=(const DictionaryItem& rhs) = default;
     bool operator<(const DictionaryItem& rhs) const {
@@ -393,7 +400,8 @@ void SdchOwner::OnDictionaryFetched(base::Time last_used,
     if (it.last_used() < stale_boundary ||
         (it.use_count() == 0 && it.last_used() < never_used_stale_boundary)) {
       stale_dictionary_list.push_back(DictionaryItem(
-          it.last_used(), it.server_hash(), it.use_count(), it.size()));
+          it.last_used(), it.server_hash(), it.use_count(), it.size(),
+          it.load_time()));

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

Shouldn't this be a lookup in the load_times_ map?  Though actually, I don't
think the value will ever be used; should the load time be in the structure at
all?

[Elly Fong-Jones, 2015/04/03 15:20:59]

Done.

       recoverable_bytes += it.size();
     }
   }
@@ -452,6 +460,7 @@ void SdchOwner::OnDictionaryFetched(base::Time last_used,
         use_counts_at_load_[stale_it->server_hash];
     RecordDictionaryEvictionOrUnload(new_uses,
                                      DICTIONARY_FATE_EVICT_FOR_DICT);
+    RecordDictionaryLifetime(stale_it->server_hash, stale_it->dictionary_size);
 
     ++stale_it;
   }
@@ -487,6 +496,7 @@ void SdchOwner::OnDictionaryFetched(base::Time last_used,
   dictionary_description->SetInteger(kDictionarySizeKey,
                                      dictionary_text.size());
   pref_dictionary_map->Set(server_hash, dictionary_description.Pass());
+  load_times_[server_hash] = clock_->Now();
 }
 
 void SdchOwner::OnDictionaryUsed(SdchManager* manager,
@@ -780,4 +790,17 @@ bool SdchOwner::IsPersistingDictionaries() const {
   return in_memory_pref_store_.get() != nullptr;
 }
 
+void SdchOwner::RecordDictionaryLifetime(const std::string& server_hash,
+                                         size_t size) {
+  if (load_times_.count(server_hash) == 0)

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

When will this happen?  Shouldn't this be a DCHECK?  Or the "return;" a
"NOTREACHED(); return;"?

[Elly Fong-Jones, 2015/04/03 15:21:00]

Done.

+    return;
+  base::Time now = clock_->Now();
+  base::TimeDelta dict_lifetime = now - load_times_[server_hash];
+  base::TimeDelta process_lifetime = now -
+      base::CurrentProcessInfo::CreationTime();
+  int64 frac = (size * dict_lifetime.InMilliseconds()) /
+      process_lifetime.InMilliseconds();
+  UMA_HISTOGRAM_COUNTS("Sdch3.TimeWeightedMemoryUse", frac);
+}
+
 }  // namespace net