SDCH: add TimeWeightedMemoryUse

net/sdch/sdch_owner.cc

 // Copyright 2014 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 "net/sdch/sdch_owner.h"
 
 #include "base/bind.h"
 #include "base/debug/alias.h"
 #include "base/logging.h"
 #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"
 #include "net/base/sdch_manager.h"
 #include "net/base/sdch_net_log_params.h"
 
 namespace net {
 
 namespace {
 
@@ skipping 147 matching lines @@
   explicit DictionaryPreferenceIterator(WriteablePrefStore* pref_store);
 
   bool IsAtEnd() const;
   void Advance();
 
   const std::string& server_hash() const { return server_hash_; }
   const GURL url() const { return url_; }
   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();
 
   std::string server_hash_;
   GURL url_;
   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_;
 };
 
 DictionaryPreferenceIterator::DictionaryPreferenceIterator(
     WriteablePrefStore* pref_store)
     : dictionary_iterator_(*GetPersistentStoreDictionaryMap(pref_store)) {
   if (!IsAtEnd())
     LoadDictionaryOrDie();
 }
@@ skipping 104 matching lines @@
   CHECK(manager_.get());
 #endif
 
   for (DictionaryPreferenceIterator it(pref_store_); !it.IsAtEnd();
        it.Advance()) {
     int new_uses = it.use_count() - use_counts_at_load_[it.server_hash()];
     DictionaryFate fate = IsPersistingDictionaries() ?
                           DICTIONARY_FATE_UNLOAD_FOR_DESTRUCTION :
                           DICTIONARY_FATE_EVICT_FOR_DESTRUCTION;
     RecordDictionaryEvictionOrUnload(new_uses, fate);
+    RecordDictionaryLifetime(it.server_hash(), it.size());
   }
   manager_->RemoveObserver(this);
 
   // This object only observes the external store during loading,
   // i.e. before it's made the default preferences store.
   if (external_pref_store_)
     external_pref_store_->RemoveObserver(this);
 
 #if defined(OS_CHROMEOS)
   destroyed_ = 0xdeadbeef;
@@ skipping 21 matching lines @@
 void SdchOwner::OnDictionaryFetched(base::Time last_used,
                                     int use_count,
                                     const std::string& dictionary_text,
                                     const GURL& dictionary_url,
                                     const net::BoundNetLog& net_log) {
   struct DictionaryItem {
     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 {
       return last_used < rhs.last_used;
     }
   };
 
 #if defined(OS_CHROMEOS)
   // For debugging http://crbug.com/454198; remove when resolved.
   CHECK_EQ(0u, destroyed_);
@@ skipping 12 matching lines @@
       now - base::TimeDelta::FromHours(kFreshnessLifetimeHours));
   // Dictionaries that have never been used and are from before
   // |never_used_stale_boundary| are candidates for eviction if necessary.
   base::Time never_used_stale_boundary(
       now - base::TimeDelta::FromHours(kNeverUsedFreshnessLifetimeHours));
   for (DictionaryPreferenceIterator it(pref_store_); !it.IsAtEnd();
        it.Advance()) {
     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();
     }
   }
 
 #if defined(OS_CHROMEOS)
   // For debugging http://crbug.com/454198; remove when resolved.
   CHECK_EQ(0u, destroyed_);
   CHECK(clock_.get());
 #endif
 
@@ skipping 38 matching lines @@
     bool success = pref_dictionary_map->RemoveWithoutPathExpansion(
         stale_it->server_hash, nullptr);
     DCHECK(success);
 
     avail_bytes += stale_it->dictionary_size;
 
     int new_uses = stale_it->use_count -
         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;
   }
   DCHECK_GE(avail_bytes, dictionary_text.size());
 
   RecordDictionaryFate(
       // Distinguish between loads triggered by network responses and
       // loads triggered by persistence.
       last_used.is_null() ? DICTIONARY_FATE_ADD_RESPONSE_TRIGGERED
                           : DICTIONARY_FATE_ADD_PERSISTENCE_TRIGGERED);
@@ skipping 15 matching lines @@
   // Record the addition in the pref store.
   scoped_ptr<base::DictionaryValue> dictionary_description(
       new base::DictionaryValue());
   dictionary_description->SetString(kDictionaryUrlKey, dictionary_url.spec());
   dictionary_description->SetDouble(kDictionaryLastUsedKey,
                                     last_used.ToDoubleT());
   dictionary_description->SetInteger(kDictionaryUseCountKey, use_count);
   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,
                                  const std::string& server_hash) {
   base::Time now(clock_->Now());
   base::DictionaryValue* pref_dictionary_map =
       GetPersistentStoreDictionaryMap(pref_store_);
   ScopedPrefNotifier scoped_pref_notifier(pref_store_);
 
   base::Value* value = nullptr;
@@ skipping 273 matching lines @@
                              base::Time::FromDoubleT(last_used), use_count));
   }
 
   return true;
 }
 
 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