Add a minimum byte count to metrics log serialization

chrome/browser/metrics/metrics_log_serializer.cc

Index: chrome/browser/metrics/metrics_log_serializer.cc
diff --git a/chrome/browser/metrics/metrics_log_serializer.cc b/chrome/browser/metrics/metrics_log_serializer.cc
index 3369f85a7a630d2fb4048732e554a3b5d0a8c320..0fc0c57a07c282901690a2ea7bc8366bbc7f2427 100644
--- a/chrome/browser/metrics/metrics_log_serializer.cc
+++ b/chrome/browser/metrics/metrics_log_serializer.cc
@@ -30,6 +30,12 @@ const size_t kMaxInitialLogsPersisted = 20;
 // ongoing_log_ at startup).
 const size_t kMaxOngoingLogsPersisted = 8;
 
+// The number of bytes each of initial and ongoing logs that must be reached
+// before the count-based limits above will be enforced. This ensures that a
+// reasonable amount of history will be stored even if there is a long series
+// of very small logs.
+const size_t kMinStorageBytesPerLogType = 300000;
+
 // We append (2) more elements to persisted lists: the size of the list and a
 // checksum of the elements.
 const size_t kChecksumEntryCount = 2;
@@ -94,11 +100,13 @@ void MetricsLogSerializer::SerializeLogs(
 
   // Write the XML version.
   ListPrefUpdate update_xml(local_state, pref_xml);
-  WriteLogsToPrefList(logs, true, max_store_count, update_xml.Get());
+  WriteLogsToPrefList(logs, true, max_store_count, kMinStorageBytesPerLogType,
+                      update_xml.Get());
 
   // Write the protobuf version.
   ListPrefUpdate update_proto(local_state, pref_proto);
-  WriteLogsToPrefList(logs, false, max_store_count, update_proto.Get());
+  WriteLogsToPrefList(logs, false, max_store_count, kMinStorageBytesPerLogType,
+                      update_proto.Get());
 }
 
 void MetricsLogSerializer::DeserializeLogs(
@@ -131,16 +139,36 @@ void MetricsLogSerializer::DeserializeLogs(
 void MetricsLogSerializer::WriteLogsToPrefList(
     const std::vector<MetricsLogManager::SerializedLog>& local_list,
     bool is_xml,
-    size_t max_list_size,
+    size_t list_length_limit,
+    size_t byte_limit,
     base::ListValue* list) {
+  // One of the limit arguments must be non-zero.
+  DCHECK(list_length_limit > 0 || byte_limit > 0);
+
   list->Clear();
-  size_t start = 0;
-  if (local_list.size() > max_list_size)
-    start = local_list.size() - max_list_size;
-  DCHECK_LE(start, local_list.size());
-  if (local_list.size() <= start)
+  if (local_list.size() == 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 (local_list.size() > list_length_limit) {
+    start = local_list.size();
+    size_t bytes_used = 0;
+    for (std::vector<MetricsLogManager::SerializedLog>::const_reverse_iterator
+         it = local_list.rbegin(); it != local_list.rend(); ++it) {
+      // TODO(isherman): Always uses XML length so both formats of a given log
+      // will be saved; switch to proto once that's the primary format.
+      size_t log_size = it->xml.length();
+      if (bytes_used >= byte_limit &&
+          (local_list.size() - start) >= list_length_limit)

[jar (doing other things), 2012/05/15 18:27:40]

You probably want ">" rather than ">=" now.

Currently, if list_length_limit == 0, then we could break on the first pass...
and get to line 170, and violate the DCHECK.

I guess this gets back to the meaning of "limit".  

The alternative (in my other comment) is to remove the DCHECK, and allow a call
where we sometimes choose to not persist any logs.  I think that is a very
plausible scenario on a low memory system anyway.

[stuartmorgan, 2012/05/15 19:02:58]

No, because it's an &&. To satisfy both conditions on the first pass, both
limits would have to be 0, and that's a DCHECK at the start of the method.
Since we always have at least one log to save, this would guarantee loss of
metrics on every single shutdown (or app termination on mobile). I'm having a
hard time imagining a device with so little persistant storage that we would
consider always throwing away metrics in normal use, so I'd rather not design
for it.

+        break;
+      bytes_used += log_size;
+      --start;
+    }
+  }
+  DCHECK_LT(start, local_list.size());
+
   // Store size at the beginning of the list.
   list->Append(Value::CreateIntegerValue(local_list.size() - start));