Skip oversized logs when persisting logs instead of discarding them.

components/metrics/persisted_logs.cc

Index: components/metrics/persisted_logs.cc
diff --git a/components/metrics/persisted_logs.cc b/components/metrics/persisted_logs.cc
index 17d6aa7dfd53754b9895f216bda9a5e7ad335fbf..825764ca28ef1b4687712f44366a59e9bb91ec1c 100644
--- a/components/metrics/persisted_logs.cc
+++ b/components/metrics/persisted_logs.cc
@@ -86,7 +86,7 @@ PersistedLogs::PersistedLogs(PrefService* local_state,
       old_pref_name_(old_pref_name),
       min_log_count_(min_log_count),
       min_log_bytes_(min_log_bytes),
-      max_log_size_(max_log_size),
+      max_log_size_(max_log_size ? max_log_size : static_cast<size_t>(-1)),

[Alexei Svitkine (slow), 2014/07/15 18:52:59]

Can you use SIZE_MAX instead of casting -1 to a size_t? Also, please make the
check != 0, since it's not a bool or pointer.

[Steven Holte, 2014/07/15 20:23:03]

Done.

       last_provisional_store_index_(-1) {
   DCHECK(local_state_);
   // One of the limit arguments must be non-zero.
@@ -95,22 +95,7 @@ PersistedLogs::PersistedLogs(PrefService* local_state,
 
 PersistedLogs::~PersistedLogs() {}
 
-void PersistedLogs::SerializeLogs() {
-  // Remove any logs that are over the serialization size limit.
-  if (max_log_size_) {
-    for (std::vector<LogHashPair>::iterator it = list_.begin();
-         it != list_.end();) {
-      size_t log_size = it->compressed_log_data.length();
-      if (log_size > max_log_size_) {
-        UMA_HISTOGRAM_COUNTS("UMA.Large Accumulated Log Not Persisted",
-                             static_cast<int>(log_size));
-        it = list_.erase(it);
-      } else {
-        ++it;
-      }
-    }
-  }
-
+void PersistedLogs::SerializeLogs() const {
   ListPrefUpdate update(local_state_, pref_name_);
   WriteLogsToPrefList(update.Get());
 
@@ -169,33 +154,39 @@ void PersistedLogs::DiscardLastProvisionalStore() {
   last_provisional_store_index_ = -1;
 }
 
-void PersistedLogs::WriteLogsToPrefList(base::ListValue* list_value) {
+void PersistedLogs::WriteLogsToPrefList(base::ListValue* list_value) const {
   list_value->Clear();
-  // Leave the list completely empty if there are no storable values.
-  if (list_.empty())
-    return;
 
-  size_t start = 0;
-  // If there are too many logs, keep the most recent logs up to the length
-  // limit, and at least to the minimum number of bytes.
-  if (list_.size() > min_log_count_) {
-    start = list_.size();
-    size_t bytes_used = 0;
-    std::vector<LogHashPair>::const_reverse_iterator end = list_.rend();
-    for (std::vector<LogHashPair>::const_reverse_iterator it = list_.rbegin();
-         it != end; ++it) {
-      const size_t log_size = it->compressed_log_data.length();
-      if (bytes_used >= min_log_bytes_ &&
-          (list_.size() - start) >= min_log_count_) {
-        break;
-      }
+  // Keep the most recent logs which are smaller than max_log_size_.

[Alexei Svitkine (slow), 2014/07/15 18:52:59]

Nit: Surround variable names in |'s.

[Steven Holte, 2014/07/15 20:23:03]

Done.

+  // We keep at least min_log_bytes_ and min_log_count_ of logs before
+  // discarding older logs.
+  size_t start = list_.size();
+  size_t saved_log_count = 0;
+  size_t bytes_used = 0;
+  for (; start > 0; --start) {
+    size_t log_size = list_[start-1].compressed_log_data.length();

[Alexei Svitkine (slow), 2014/07/15 18:52:59]

Nit: spaces around the minus.

[Steven Holte, 2014/07/15 20:23:03]

Done.

+    if (bytes_used >= min_log_bytes_ &&
+        saved_log_count >= min_log_count_) {
+      break;
+    }
+    // Oversized logs won't be persisted, so don't count them.

[Alexei Svitkine (slow), 2014/07/15 18:52:59]

I don't think the piece of code is needed, since the loop below would already
ignore them. Plus, by avoiding this and the early return below, we won't lose
the histogram for these large logs.

[Steven Holte, 2014/07/15 20:23:03]

The second loop iterates in the reverse direction from where this one stops, and
we wouldn't go back as far if we don't skip these.

[Steven Holte, 2014/07/15 20:25:47]

Also, I've now removed the early return below, since it is unnecessary, so the
histogram counts won't go missing.

+    if (log_size <= max_log_size_) {
       bytes_used += log_size;
-      --start;
+      ++saved_log_count;
     }
   }
-  DCHECK_LT(start, list_.size());
+
+  // Leave the list completely empty if there are no storable values.
+  if (saved_log_count == 0)
+    return;
 
   for (size_t i = start; i < list_.size(); ++i) {
+    size_t log_size = list_[i].compressed_log_data.length();
+    if (log_size > max_log_size_) {
+      UMA_HISTOGRAM_COUNTS("UMA.Large Accumulated Log Not Persisted",
+                             static_cast<int>(log_size));
+      continue;
+    }
     AppendBase64String(list_[i].compressed_log_data, list_value);
     AppendBase64String(list_[i].hash, list_value);
   }