Changes to upload tracked_objects data from all renderer

chrome/browser/metrics/tracking_synchronizer.cc

Index: chrome/browser/metrics/tracking_synchronizer.cc
===================================================================
--- chrome/browser/metrics/tracking_synchronizer.cc	(revision 0)
+++ chrome/browser/metrics/tracking_synchronizer.cc	(revision 0)
@@ -0,0 +1,246 @@
+// Copyright (c) 2011 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 "chrome/browser/metrics/tracking_synchronizer.h"
+
+#include "base/json/json_reader.h"
+#include "base/json/json_writer.h"
+#include "base/logging.h"
+#include "base/metrics/histogram.h"
+#include "base/threading/thread.h"
+#include "base/tracked_objects.h"
+#include "chrome/common/chrome_constants.h"
+#include "chrome/common/render_messages.h"
+#include "content/browser/browser_thread.h"
+#include "content/browser/renderer_host/render_process_host.h"
+
+using base::Time;

[jar (doing other things), 2011/10/30 08:04:19]

We probably won't use this.  It is "wall clock" time, and only used for external
visible dates and times.

[ramant (doing other things), 2011/11/01 00:52:15]

Done.

+using base::TimeDelta;
+using base::TimeTicks;
+
+namespace chrome_browser_metrics {
+
+// Negative numbers are never used as sequence numbers.  We explicitly pick a
+// negative number that is "so negative" that even when we add one (as is done
+// when we generated the next sequence number) that it will still be negative.
+// We have code that handles wrapping around on an overflow into negative
+// territory.
+static const int kNeverUsableSequenceNumber = -2;
+
+TrackingSynchronizer::TrackingSynchronizer()
+  : lock_(),
+    last_used_sequence_number_(kNeverUsableSequenceNumber),
+    // TODO(rtenneti): delete last_value_from_renderer_.
+    last_value_from_renderer_(NULL) {
+  DCHECK(tracking_synchronizer_ == NULL);
+  tracking_synchronizer_ = this;
+}
+
+TrackingSynchronizer::~TrackingSynchronizer() {
+  // Just in case we have any pending tasks, clear them out.
+  while (!outstanding_requests_.empty()) {
+    RequestContextMap::iterator it = outstanding_requests_.begin();
+    delete it->second;
+    outstanding_requests_.erase(it);
+  }
+
+  tracking_synchronizer_ = NULL;
+}
+
+// static
+TrackingSynchronizer* TrackingSynchronizer::CurrentSynchronizer() {
+  DCHECK(tracking_synchronizer_ != NULL);
+  return tracking_synchronizer_;
+}
+
+// static
+// TODO(rtenneti): Delete this method after testing.
+void TrackingSynchronizer::FetchTrackingDataSynchronously(std::string* output) {
+  TrackingSynchronizer* current_synchronizer = CurrentSynchronizer();
+
+  CallbackObject* callback_object = new CallbackObject;
+  FetchTrackingDataAsynchronously(callback_object);
+
+  // We get tracked data fro browser process immediately.
+  base::JSONWriter::Write(callback_object->value_, false, output);

[jar (doing other things), 2011/10/30 08:04:19]

To test this equally simply... you could do a PostTask wit the value you get
from the browser's version of ThreadData. That will allow you to exercise the
code that other processes will call.  I suspect you'll really use that path even
for the browser process.

[ramant (doing other things), 2011/11/01 00:52:15]

Done.

+
+  if (!current_synchronizer->last_value_from_renderer_)
+    return;
+
+  base::JSONWriter::Write(
+      current_synchronizer->last_value_from_renderer_, false, output);
+}
+
+// static
+void TrackingSynchronizer::FetchTrackingDataAsynchronously(
+    CallbackObject* callback_object) {
+  DCHECK(callback_object != NULL);
+
+  TrackingSynchronizer* current_synchronizer = CurrentSynchronizer();
+
+  if (current_synchronizer == NULL) {
+    // System teardown is happening.
+    return;
+  }
+
+  // Get the ThreadData for the browser process and send it back.
+  base::Value* value = tracked_objects::ThreadData::ToValue(0);
+  callback_object->SendData(value);
+
+  int sequence_number = current_synchronizer->RegisterAndNotifyAllProcesses(
+      callback_object);
+
+  // Post a task that would be called after waiting for wait_time.  This acts
+  // as a watchdog, to cancel the requests for non-responsive processes.
+  BrowserThread::PostDelayedTask(
+      BrowserThread::UI, FROM_HERE,
+      NewRunnableMethod(
+          current_synchronizer,
+          &TrackingSynchronizer::ForceTrackingSynchronizationDoneCallback,
+          sequence_number),
+      10000);

[jar (doing other things), 2011/10/30 08:04:19]

You may as well make this a bit longer.  There is no big carrying cost for
delaying the discard... and some browsers will be pretty darn un-responsive at
times.
I'd suggest 60 seconds (rather than the 10 seconds shown).

[ramant (doing other things), 2011/11/01 00:52:15]

Done.

+}
+
+// static
+void TrackingSynchronizer::DeserializeTrackingList(
+    int sequence_number,
+    const std::string& tracking_data) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+
+  base::Value* value =
+      base::JSONReader().JsonToValue(tracking_data, false, true);
+
+  TrackingSynchronizer* current_synchronizer = CurrentSynchronizer();
+  if (current_synchronizer == NULL)
+    return;

[jar (doing other things), 2011/10/30 08:04:19]

This would leak value.  Perhaps you should do the check before you construct the
value.

[ramant (doing other things), 2011/11/01 00:52:15]

Done.

+
+  // Send back the tracking data we have received from a process.
+  current_synchronizer->DecrementPendingProcessesAndSendData(
+      sequence_number, value);
+}
+
+int TrackingSynchronizer::RegisterAndNotifyAllProcesses(
+    CallbackObject* callback_object) {
+  // To iterate over all processes, or to send messages to the hosts, we need
+  // to be on the UI thread.
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+
+  int notification_count = 0;
+  for (RenderProcessHost::iterator it(RenderProcessHost::AllHostsIterator());
+       !it.IsAtEnd(); it.Advance())
+     ++notification_count;

[jar (doing other things), 2011/10/30 08:04:19]

This number is probably changing underneath us. I'd suggest that you count as
you send, rather than counting in advance.

[ramant (doing other things), 2011/11/01 00:52:15]

Done.

+
+  int sequence_number = GetNextAvailableSequenceNumber();
+
+  Register(sequence_number, callback_object, notification_count);

[jar (doing other things), 2011/10/30 08:04:19]

You should probably just set the count to 1 here (so it is always non-zero
during the loop below). You can then have a method which increments the
notifcation count each time just before you sent a request. Finally, after
you've sent out all the requests, you can decrement the count (using the
function that potentially discards the callback when it hits  zero.

[ramant (doing other things), 2011/11/01 00:52:15]

Done.

+
+  for (RenderProcessHost::iterator it(RenderProcessHost::AllHostsIterator());
+       !it.IsAtEnd(); it.Advance()) {
+    if (!it.GetCurrentValue()->Send(
+        new ChromeViewMsg_GetRendererTrackedData(sequence_number)))
+      DecrementPendingProcesses(sequence_number);
+  }
+
+  return sequence_number;
+}
+
+void TrackingSynchronizer::Register(int sequence_number,
+                                    CallbackObject* callback_object,
+                                    int processes_pending) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+
+  RequestContext* request = new RequestContext(
+      callback_object, sequence_number, processes_pending, TimeTicks::Now());
+  {
+    base::AutoLock auto_lock(lock_);

[jar (doing other things), 2011/10/30 08:04:19]

So long as we only access this on the UI thread, we won't need a lock.

[ramant (doing other things), 2011/11/01 00:52:15]

Done.

+    outstanding_requests_[sequence_number] = request;
+  }
+}
+
+void TrackingSynchronizer::DecrementPendingProcessesAndSendData(
+    int sequence_number, base::Value* value) {
+  RequestContext* request = NULL;
+  bool completed = false;
+

[jar (doing other things), 2011/10/30 08:04:19]

DCHECK on UI thread.

[ramant (doing other things), 2011/11/01 00:52:15]

Done.

+  {
+    base::AutoLock auto_lock(lock_);
+    RequestContextMap::iterator it =
+        outstanding_requests_.find(sequence_number);
+    if (it == outstanding_requests_.end())
+      return;
+
+    request = it->second;
+
+    DCHECK(sequence_number == request->sequence_number_);
+    if (--request->processes_pending_ <= 0)
+      completed = true;
+
+    // TODO(rtenneti): Delete last_value_from_renderer_ after testing.
+    if (value)
+      last_value_from_renderer_ = value;
+  }
+
+  if (request) {
+    // TODO(rtenneti): SendData is called on IO thread. Should
+    // |callback_object_| do a PostTask to UI thread??

[jar (doing other things), 2011/10/30 08:04:19]

yeah... we should have bounced over to the UI thread at the start of this method
(probably have a static version that runs on the IO thread but bounces all
messages to this method)... so we'd be on the UI thread now.

[ramant (doing other things), 2011/11/01 00:52:15]

Done.

+    request->callback_object_->SendData(value);

[jar (doing other things), 2011/10/30 08:04:19]

You need to do this only if value is non-null.  On line 195 you send NULL down
this path... and you just need to avoid the send data.

I think some of that logic might change... but the NULL check seems reasonable.

[ramant (doing other things), 2011/11/01 00:52:15]

Done.

+  }
+
+  if (completed)
+    ForceTrackingSynchronizationDoneCallback(sequence_number);
+}
+
+void TrackingSynchronizer::DecrementPendingProcesses(int sequence_number) {
+  DecrementPendingProcessesAndSendData(sequence_number, NULL);
+}
+
+void TrackingSynchronizer::ForceTrackingSynchronizationDoneCallback(
+    int sequence_number) {
+  TimeTicks started;
+  int unresponsive_processes;
+  {
+    base::AutoLock lock(lock_);
+    RequestContextMap::iterator it =
+        outstanding_requests_.find(sequence_number);
+    if (it == outstanding_requests_.end())
+      return;
+
+    RequestContext* request = it->second;
+
+    DCHECK(sequence_number == request->sequence_number_);
+
+    started = request->request_start_time_;
+    unresponsive_processes = request->processes_pending_;
+
+    delete it->second;
+    outstanding_requests_.erase(it);
+  }
+
+  UMA_HISTOGRAM_COUNTS("Tracking.ProcessNotRespondingAsynchronous",
+                       unresponsive_processes);
+  if (!unresponsive_processes) {
+    UMA_HISTOGRAM_TIMES("Tracking.FetchProcessTrackingsAsynchronously",
+                        TimeTicks::Now() - started);

[jar (doing other things), 2011/10/30 08:04:19]

This duration should almost always be your 10second number.  I think the first
histogram is more helpful (line 220).

[ramant (doing other things), 2011/11/01 00:52:15]

Deleted this histogram.
Done.

+  }
+}
+
+int TrackingSynchronizer::GetNextAvailableSequenceNumber() {
+  base::AutoLock auto_lock(lock_);
+  ++last_used_sequence_number_;
+  // Watch out for wrapping to a negative number.
+  if (last_used_sequence_number_ < 0) {
+    // Bypass the reserved number, which is used when a process spontaneously
+    // decides to send some tracking data.
+    last_used_sequence_number_ =
+        chrome::kTrackingSynchronizerReservedSequenceNumber + 1;
+  }
+  DCHECK_NE(last_used_sequence_number_,
+            chrome::kTrackingSynchronizerReservedSequenceNumber);
+  return last_used_sequence_number_;
+}
+
+// static
+TrackingSynchronizer* TrackingSynchronizer::tracking_synchronizer_ = NULL;
+
+}  // namespace chrome_browser_metrics
Property changes on: chrome\browser\metrics\tracking_synchronizer.cc
___________________________________________________________________
Added: svn:executable
   + *