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..2fd62e355dadfdf58dde5a8f609088b23f502fa9 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"
@@ -20,43 +21,6 @@ namespace net {
 
 namespace {
 
-enum DictionaryFate {
-  // A Get-Dictionary header wasn't acted on.
-  DICTIONARY_FATE_GET_IGNORED = 1,
-
-  // A fetch was attempted, but failed.
-  // TODO(rdsmith): Actually record this case.
-  DICTIONARY_FATE_FETCH_FAILED = 2,
-
-  // A successful fetch was dropped on the floor, no space.
-  DICTIONARY_FATE_FETCH_IGNORED_NO_SPACE = 3,
-
-  // A successful fetch was refused by the SdchManager.
-  DICTIONARY_FATE_FETCH_MANAGER_REFUSED = 4,
-
-  // A dictionary was successfully added based on
-  // a Get-Dictionary header in a response.
-  DICTIONARY_FATE_ADD_RESPONSE_TRIGGERED = 5,
-
-  // A dictionary was evicted by an incoming dict.
-  DICTIONARY_FATE_EVICT_FOR_DICT = 6,
-
-  // A dictionary was evicted by memory pressure.
-  DICTIONARY_FATE_EVICT_FOR_MEMORY = 7,
-
-  // A dictionary was evicted on destruction.
-  DICTIONARY_FATE_EVICT_FOR_DESTRUCTION = 8,
-
-  // A dictionary was successfully added based on
-  // persistence from a previous browser revision.
-  DICTIONARY_FATE_ADD_PERSISTENCE_TRIGGERED = 9,
-
-  // A dictionary was unloaded on destruction, but is still present on disk.
-  DICTIONARY_FATE_UNLOAD_FOR_DESTRUCTION = 10,
-
-  DICTIONARY_FATE_MAX = 11
-};
-
 enum PersistenceFailureReason {
   // File didn't exist; is being created.
   PERSISTENCE_FAILURE_REASON_NO_FILE = 1,
@@ -77,25 +41,11 @@ const int kFreshnessLifetimeHours = 24;
 // Dictionaries that have never been used only stay fresh for one hour.
 const int kNeverUsedFreshnessLifetimeHours = 1;
 
-void RecordDictionaryFate(enum DictionaryFate fate) {
-  UMA_HISTOGRAM_ENUMERATION("Sdch3.DictionaryFate", fate, DICTIONARY_FATE_MAX);
-}
-
 void RecordPersistenceFailure(PersistenceFailureReason failure_reason) {
   UMA_HISTOGRAM_ENUMERATION("Sdch3.PersistenceFailureReason", failure_reason,
                             PERSISTENCE_FAILURE_REASON_MAX);
 }
 
-void RecordDictionaryEvictionOrUnload(int use_count, DictionaryFate fate) {
-  DCHECK(fate == DICTIONARY_FATE_EVICT_FOR_DICT ||
-         fate == DICTIONARY_FATE_EVICT_FOR_MEMORY ||
-         fate == DICTIONARY_FATE_EVICT_FOR_DESTRUCTION ||
-         fate == DICTIONARY_FATE_UNLOAD_FOR_DESTRUCTION);
-
-  UMA_HISTOGRAM_COUNTS_100("Sdch3.DictionaryUseCount", use_count);
-  RecordDictionaryFate(fate);
-}
-
 // Schema specifications and access routines.
 
 // The persistent prefs store is conceptually shared with any other network
@@ -266,6 +216,28 @@ const size_t SdchOwner::kMaxTotalDictionarySize = 20 * 1000 * 1000;
 // amount of space available in storage.
 const size_t SdchOwner::kMinSpaceForDictionaryFetch = 50 * 1000;
 
+void SdchOwner::RecordDictionaryFate(enum DictionaryFate fate) {
+  UMA_HISTOGRAM_ENUMERATION("Sdch3.DictionaryFate", fate, DICTIONARY_FATE_MAX);
+}
+
+void SdchOwner::RecordDictionaryEvictionOrUnload(const std::string& server_hash,
+                                                 size_t size,
+                                                 int use_count,
+                                                 DictionaryFate fate) {
+  DCHECK(fate == DICTIONARY_FATE_EVICT_FOR_DICT ||
+         fate == DICTIONARY_FATE_EVICT_FOR_MEMORY ||
+         fate == DICTIONARY_FATE_EVICT_FOR_DESTRUCTION ||
+         fate == DICTIONARY_FATE_UNLOAD_FOR_DESTRUCTION);
+
+  UMA_HISTOGRAM_COUNTS_100("Sdch3.DictionaryUseCount", use_count);
+  RecordDictionaryFate(fate);
+
+  DCHECK(load_times_.count(server_hash) == 1);

[Randy Smith (Not in Mondays), 2015/04/03 20:36:51]

If you're going to have this DCHECK, that suggests to me that you should also
delete the entry in load_times_ at the end of this function.  Otherwise the
DCHECK() isn't checking anything if a dictionary is re-loaded.

(Just a suggestion, may be irrelevant because SdchDictionaryFetcher won't
re-fetch the same dictionary without a clear, but I'd consider that a bug if
it's true and that behavior isn't one SdchOwner should count on anyway.  But
it's just a DCHECK.)

+  base::Time now = clock_->Now();
+  base::TimeDelta dict_lifetime = now - load_times_[server_hash];
+  consumed_byte_seconds_.push_back(size * dict_lifetime.InMilliseconds());
+}
+
 SdchOwner::SdchOwner(SdchManager* sdch_manager, URLRequestContext* context)
     : manager_(sdch_manager->GetWeakPtr()),
       fetcher_(new SdchDictionaryFetcher(context)),
@@ -309,7 +281,8 @@ SdchOwner::~SdchOwner() {
     DictionaryFate fate = IsPersistingDictionaries() ?
                           DICTIONARY_FATE_UNLOAD_FOR_DESTRUCTION :
                           DICTIONARY_FATE_EVICT_FOR_DESTRUCTION;
-    RecordDictionaryEvictionOrUnload(new_uses, fate);
+    RecordDictionaryEvictionOrUnload(it.server_hash(), it.size(), new_uses,
+                                     fate);
   }
   manager_->RemoveObserver(this);
 
@@ -318,6 +291,14 @@ SdchOwner::~SdchOwner() {
   if (external_pref_store_)
     external_pref_store_->RemoveObserver(this);
 
+  base::Time creation_time = base::CurrentProcessInfo::CreationTime();

[Randy Smith (Not in Mondays), 2015/04/03 20:36:51]

I'm still inclined to think this should just be the lifetime of the SdchOwner;
that seems strictly better than process lifetime.  WDYT?

+  int64 process_lifetime =
+      (clock_->Now() - creation_time).InMilliseconds();
+  for (auto& val : consumed_byte_seconds_) {
+    UMA_HISTOGRAM_COUNTS("Sdch3.TimeWeightedMemoryUse",
+                         val / process_lifetime);
+  }
+
 #if defined(OS_CHROMEOS)
   destroyed_ = 0xdeadbeef;
 #endif
@@ -450,7 +431,9 @@ void SdchOwner::OnDictionaryFetched(base::Time last_used,
 
     int new_uses = stale_it->use_count -
         use_counts_at_load_[stale_it->server_hash];
-    RecordDictionaryEvictionOrUnload(new_uses,
+    RecordDictionaryEvictionOrUnload(stale_it->server_hash,
+                                     stale_it->dictionary_size,
+                                     new_uses,
                                      DICTIONARY_FATE_EVICT_FOR_DICT);
 
     ++stale_it;
@@ -487,6 +470,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,
@@ -719,7 +703,9 @@ void SdchOwner::OnMemoryPressure(
   for (DictionaryPreferenceIterator it(pref_store_); !it.IsAtEnd();
        it.Advance()) {
     int new_uses = it.use_count() - use_counts_at_load_[it.server_hash()];
-    RecordDictionaryEvictionOrUnload(new_uses,
+    RecordDictionaryEvictionOrUnload(it.server_hash(),
+                                     it.size(),
+                                     new_uses,
                                      DICTIONARY_FATE_EVICT_FOR_MEMORY);
   }