Introducing phased profiling framework

components/metrics/profiler/profiler_metrics_provider.cc

Index: components/metrics/profiler/profiler_metrics_provider.cc
diff --git a/components/metrics/profiler/profiler_metrics_provider.cc b/components/metrics/profiler/profiler_metrics_provider.cc
index b10dc29a2e7bb706545fdab86f63b32e25398c4a..43196cd580ef01bc3ed272bcebbc51c3db189338 100644
--- a/components/metrics/profiler/profiler_metrics_provider.cc
+++ b/components/metrics/profiler/profiler_metrics_provider.cc
@@ -12,9 +12,6 @@
 #include "components/metrics/metrics_log.h"
 #include "components/nacl/common/nacl_process_type.h"
 #include "components/variations/variations_associated_data.h"
-#include "content/public/common/process_type.h"
-
-using tracked_objects::ProcessDataSnapshot;
 
 namespace metrics {
 
@@ -77,33 +74,31 @@ std::string NormalizeFileName(const std::string& file_name) {
   return offset != std::string::npos ? file_name.substr(offset + 1) : file_name;
 }
 
-void WriteProfilerData(const ProcessDataSnapshot& profiler_data,
-                       int process_type,
-                       ProfilerEventProto* performance_profile) {
-  for (std::vector<tracked_objects::TaskSnapshot>::const_iterator it =
-           profiler_data.tasks.begin();
-       it != profiler_data.tasks.end();
-       ++it) {
-    const tracked_objects::DeathDataSnapshot& death_data = it->death_data;
+void WriteProfilerData(
+    const tracked_objects::ProcessDataPhaseSnapshot& process_data_phase,
+    base::ProcessId process_id,
+    content::ProcessType process_type,
+    ProfilerEventProto* performance_profile) {
+  for (const auto& task : process_data_phase.tasks) {
+    const tracked_objects::DeathDataSnapshot& death_data = task.death_data;
     ProfilerEventProto::TrackedObject* tracked_object =
         performance_profile->add_tracked_object();
     tracked_object->set_birth_thread_name_hash(
-        MetricsLog::Hash(MapThreadName(it->birth.thread_name)));
+        MetricsLog::Hash(MapThreadName(task.birth.thread_name)));
     tracked_object->set_exec_thread_name_hash(
-        MetricsLog::Hash(MapThreadName(it->death_thread_name)));
+        MetricsLog::Hash(MapThreadName(task.death_thread_name)));
     tracked_object->set_source_file_name_hash(
-        MetricsLog::Hash(NormalizeFileName(
-            it->birth.location.file_name)));
+        MetricsLog::Hash(NormalizeFileName(task.birth.location.file_name)));
     tracked_object->set_source_function_name_hash(
-        MetricsLog::Hash(it->birth.location.function_name));
-    tracked_object->set_source_line_number(it->birth.location.line_number);
+        MetricsLog::Hash(task.birth.location.function_name));
+    tracked_object->set_source_line_number(task.birth.location.line_number);
     tracked_object->set_exec_count(death_data.count);
     tracked_object->set_exec_time_total(death_data.run_duration_sum);
     tracked_object->set_exec_time_sampled(death_data.run_duration_sample);
     tracked_object->set_queue_time_total(death_data.queue_duration_sum);
     tracked_object->set_queue_time_sampled(death_data.queue_duration_sample);
     tracked_object->set_process_type(AsProtobufProcessType(process_type));
-    tracked_object->set_process_id(profiler_data.process_id);
+    tracked_object->set_process_id(process_id);
   }
 }
 
@@ -117,12 +112,11 @@ bool IsCellularEnabledByExperiment() {
 
 }  // namespace
 
-ProfilerMetricsProvider::ProfilerMetricsProvider() : has_profiler_data_(false) {
+ProfilerMetricsProvider::ProfilerMetricsProvider() {
 }
 
 ProfilerMetricsProvider::ProfilerMetricsProvider(
-    const base::Callback<void(bool*)>& cellular_callback)
-    : has_profiler_data_(false), cellular_callback_(cellular_callback) {
+    const base::Callback<void(bool*)>& cellular_callback) {

[Ilya Sherman, 2015/03/17 23:57:38]

Why did you drop the cellular_callback_ initialization code here?

[vadimt, 2015/03/19 00:20:44]

Merge :( Horrible :(

 }
 
 ProfilerMetricsProvider::~ProfilerMetricsProvider() {
@@ -130,21 +124,26 @@ ProfilerMetricsProvider::~ProfilerMetricsProvider() {
 
 void ProfilerMetricsProvider::ProvideGeneralMetrics(
     ChromeUserMetricsExtension* uma_proto) {
-  if (!has_profiler_data_)
-    return;
-
   DCHECK_EQ(tracked_objects::TIME_SOURCE_TYPE_WALL_TIME,
             tracked_objects::GetTimeSourceType());
 
   DCHECK_EQ(0, uma_proto->profiler_event_size());
-  ProfilerEventProto* profile = uma_proto->add_profiler_event();
-  profile->Swap(&profiler_event_cache_);
-  has_profiler_data_ = false;
+
+  for (const auto& event : profiler_events_cache_) {
+    uma_proto->add_profiler_event()->CopyFrom(event.second);

[Ilya Sherman, 2015/03/17 23:57:38]

Can you swap rather than copying?

[vadimt, 2015/03/19 00:20:44]

I can. However, why should I do this? Performance?

Note that I empty the whole profiler_events_cache_ map afterwards.

[Ilya Sherman, 2015/03/19 01:00:51]

Yes, for performance reasons.  It's fine to copy if need be, but Swap() is just
as easy to use, and is more efficient.  Might as well use it here.

[vadimt, 2015/03/19 18:01:47]

Done.

+  }
+
+  profiler_events_cache_.clear();
 }
 
 void ProfilerMetricsProvider::RecordProfilerData(
-    const tracked_objects::ProcessDataSnapshot& process_data,
-    int process_type) {
+    const tracked_objects::ProcessDataPhaseSnapshot& process_data_phase,
+    base::ProcessId process_id,
+    content::ProcessType process_type,
+    int profiling_phase,
+    const base::TimeDelta& phase_start,
+    const base::TimeDelta& phase_finish,
+    const ProfilerEvents& past_events) {
   if (IsCellularConnection() && IsCellularEnabledByExperiment())
     return;
   if (tracked_objects::GetTimeSourceType() !=
@@ -153,11 +152,22 @@ void ProfilerMetricsProvider::RecordProfilerData(
     return;
   }
 
-  has_profiler_data_ = true;
-  profiler_event_cache_.set_profile_version(
-      ProfilerEventProto::VERSION_STARTUP_PROFILE);
-  profiler_event_cache_.set_time_source(ProfilerEventProto::WALL_CLOCK_TIME);
-  WriteProfilerData(process_data, process_type, &profiler_event_cache_);
+  const bool new_phase = !ContainsKey(profiler_events_cache_, profiling_phase);

[Ilya Sherman, 2015/03/17 23:57:38]

nit: I'd inline this code, rather than caching the result in the |new_phase|
variable.

[vadimt, 2015/03/19 00:20:44]

The next line will unconditionally add the element to the map, and the same
expression will be always false. So, the semantics will change.

+  ProfilerEventProto* profiler_event = &profiler_events_cache_[profiling_phase];
+
+  if (new_phase) {
+    profiler_event->set_profile_version(
+        ProfilerEventProto::VERSION_SPLIT_PROFILE);
+    profiler_event->set_time_source(ProfilerEventProto::WALL_CLOCK_TIME);
+    profiler_event->set_profiling_start_ms(phase_start.InMilliseconds());
+    profiler_event->set_profiling_finish_ms(phase_finish.InMilliseconds());
+    for (const auto& event : past_events) {
+      profiler_event->add_past_session_events(event);

[Ilya Sherman, 2015/03/17 23:57:38]

This method name should be "add_past_session_event", which suggests that the
proto field is misnamed.

[vadimt, 2015/03/19 00:20:44]

Done.

+    }
+  }
+
+  WriteProfilerData(process_data_phase, process_id, process_type,
+                    profiler_event);
 }
 
 bool ProfilerMetricsProvider::IsCellularConnection() {