Skip oversized logs when persisting logs instead of discarding them.

components/metrics/persisted_logs.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 "components/metrics/persisted_logs.h"
 
 #include <string>
 
 #include "base/base64.h"
 #include "base/md5.h"
@@ skipping 68 matching lines @@
                              const char* pref_name,
                              const char* old_pref_name,
                              size_t min_log_count,
                              size_t min_log_bytes,
                              size_t max_log_size)
     : local_state_(local_state),
       pref_name_(pref_name),
       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.
   DCHECK(min_log_count_ > 0 || min_log_bytes_ > 0);
 }
 
 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());
 
   // Clear the old pref now that we've written to the new one.
   // TODO(asvitkine): Remove the old pref in M39.
   local_state_->ClearPref(old_pref_name_);
 }
 
 PersistedLogs::LogReadStatus PersistedLogs::DeserializeLogs() {
   // First, try reading from old pref. If it's empty, read from the new one.
@@ skipping 38 matching lines @@
 }
 
 void PersistedLogs::DiscardLastProvisionalStore() {
   if (last_provisional_store_index_ == -1)
     return;
   DCHECK_LT(static_cast<size_t>(last_provisional_store_index_), list_.size());
   list_.erase(list_.begin() + last_provisional_store_index_);
   last_provisional_store_index_ = -1;
 }
 
-void PersistedLogs::WriteLogsToPrefList(base::ListValue* list_value) {
+void PersistedLogs::WriteLogsToPrefList(base::ListValue* list_value) const {
   list_value->Clear();
+
+  // 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;
+      ++saved_log_count;
+    }
+  }
+
   // Leave the list completely empty if there are no storable values.
-  if (list_.empty())
+  if (saved_log_count == 0)
     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;
-      }
-      bytes_used += log_size;
-      --start;
+  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;
     }
-  }
-  DCHECK_LT(start, list_.size());
-
-  for (size_t i = start; i < list_.size(); ++i) {
     AppendBase64String(list_[i].compressed_log_data, list_value);
     AppendBase64String(list_[i].hash, list_value);
   }
 }
 
 PersistedLogs::LogReadStatus PersistedLogs::ReadLogsFromPrefList(
     const base::ListValue& list_value) {
   if (list_value.empty())
     return MakeRecallStatusHistogram(LIST_EMPTY);
 
@@ skipping 80 matching lines @@
     return MakeRecallStatusHistogram(CHECKSUM_STRING_CORRUPTION);
   }
   if (recovered_md5 != base::MD5DigestToBase16(digest)) {
     list_.clear();
     return MakeRecallStatusHistogram(CHECKSUM_CORRUPTION);
   }
   return MakeRecallStatusHistogram(RECALL_SUCCESS);
 }
 
 }  // namespace metrics