Avoid emptry tracing callbacks when possible.

base/trace_event/trace_event_impl.cc

 // Copyright (c) 2012 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 "base/trace_event/trace_event_impl.h"
 
 #include <algorithm>
 #include <cmath>
 
 #include "base/base_switches.h"
@@ skipping 1745 matching lines @@
 // Usually it runs on a different thread.
 void TraceLog::ConvertTraceEventsToTraceFormat(
     scoped_ptr<TraceBuffer> logged_events,
     const TraceLog::OutputCallback& flush_output_callback) {
 
   if (flush_output_callback.is_null())
     return;
 
   // The callback need to be called at least once even if there is no events
   // to let the caller know the completion of flush.
-  bool has_more_events = true;
-  do {
-    scoped_refptr<RefCountedString> json_events_str_ptr =
-        new RefCountedString();
-
-    while (json_events_str_ptr->size() < kTraceEventBufferSizeInBytes) {
-      const TraceBufferChunk* chunk = logged_events->NextChunk();
-      has_more_events = chunk != NULL;
-      if (!chunk)
-        break;
-      for (size_t j = 0; j < chunk->size(); ++j) {
-        if (json_events_str_ptr->size())
-          json_events_str_ptr->data().append(",\n");
-        chunk->GetEventAt(j)->AppendAsJSON(&(json_events_str_ptr->data()));
+  scoped_refptr<RefCountedString> json_events_str_ptr = new RefCountedString();
+  while (const TraceBufferChunk* chunk = logged_events->NextChunk()) {
+    for (size_t j = 0; j < chunk->size(); ++j) {
+      size_t size = json_events_str_ptr->size();
+      if (size > kTraceEventBufferSizeInBytes) {
+        flush_output_callback.Run(json_events_str_ptr, true);
+        json_events_str_ptr = new RefCountedString();
+      } else if (size) {
+        json_events_str_ptr->data().append(",\n");
       }
+      chunk->GetEventAt(j)->AppendAsJSON(&(json_events_str_ptr->data()));
     }
-    flush_output_callback.Run(json_events_str_ptr, has_more_events);
-  } while (has_more_events);
+  }
+  flush_output_callback.Run(json_events_str_ptr, false);

[dsinclair, 2015/06/11 13:54:39]

Doesn't this new code do the same as the old code?

If logged_events->NextChunk returns nullptr then we will flush_output_callback
with an empty json_events_str_ptr and false which was the same problem as
previously, no?

[hubbe, 2015/06/11 17:30:01]

The new code will only create an empty callback if there is no data at all,
which is consistent with the comment. The existing code can produce callbacks
like:

   DATA, true
   EMPTY, true
   DATA, true
   DATA, false

The comment doesn't explicitly say that won't happen, but it was not what I
expected, which is why the test broke.

[dsinclair, 2015/06/11 19:19:29]

Right, but I don't see how the old code could produce the EMPTY, true case with
more data afterwards? The inner while should keep us looping through the chunks
until either our buffer is full or we've processed them all. How do we end up
breaking out of the while when there are more chunks (in order for
has_more_events to be true) and we also haven't got size() > 
kTraceEventBufferSizeInBytes?

[hubbe, 2015/06/11 21:39:58]

Apparently, it's been too long since I thought about this, because my
description of the problem was not accurate. The sequence you get is:

  DATA, true
  ....
  DATA, true
  EMPTY, false

Because at the time we flush one buffer, we don't know that
it's the last one, so has_more_events is still true.

[dsinclair, 2015/06/12 13:48:35]

Ah, that makes a lot more sense, I can see how that would happen.

Thanks a lot for explaining this to me.

 }
 
 void TraceLog::FinishFlush(int generation) {
   scoped_ptr<TraceBuffer> previous_logged_events;
   OutputCallback flush_output_callback;
 
   if (!CheckGeneration(generation))
     return;
 
   {
@@ skipping 847 matching lines @@
 }
 
 ScopedTraceBinaryEfficient::~ScopedTraceBinaryEfficient() {
   if (*category_group_enabled_) {
     TRACE_EVENT_API_UPDATE_TRACE_EVENT_DURATION(category_group_enabled_,
                                                 name_, event_handle_);
   }
 }
 
 }  // namespace trace_event_internal