Introducing phased profiling framework

Introducing phased profiling framework.

TrackingSynchronizer can send a "phase completed" message to all processes. Their ThreadData's make a snapshot of task deaths and store it in a static map, associating with the number of the completed phase. They also clean the death data.

When requested to return a tasks snapshot, ThreadData returns that saved map, adding a snapshot of current task deaths as a separate "current" phase.

The rest of code was transformed to work with the map of snapshots instead of a single snapshot.

BUG=456354
Committed: https://crrev.com/379d7fe794b4a200e08ac3f23c69184edef085f7
Cr-Commit-Position: refs/heads/master@{#323151}

[vadimt, 2015-03-06 15:54:32]

vadimt@chromium.org changed reviewers:
+ asvitkine@chromium.org, isherman@chromium.org

[vadimt, 2015-03-06 15:54:32]

isherman@chromium.org, asvitkine@chromium.org, please review and pre-review.

[Alexei Svitkine (slow), 2015-03-06 20:06:40]

Still on the large side as CL goes, my preference would be to break it up into
smaller chunks still.

Anyway, here's some comments.

https://codereview.chromium.org/985773002/diff/1/base/tracked_objects.cc
File base/tracked_objects.cc (right):

https://codereview.chromium.org/985773002/diff/1/base/tracked_objects.cc#newc...
base/tracked_objects.cc:417:
&completed_phases_snapshots_.Get()[profiling_phase]);
Can this function check that this isn't calling with the same profiling phase
multiple times?

https://codereview.chromium.org/985773002/diff/1/base/tracked_objects.h
File base/tracked_objects.h (right):

https://codereview.chromium.org/985773002/diff/1/base/tracked_objects.h#newco...
base/tracked_objects.h:391: // phases, including the currect one, identified by
|current_profiling_phase|.
Nit: currect -> current

I don't understand this.

Is the assumption here that this can only be called with
|current_profiling_phase| being the current phase? What happens if you call it
with a different value? Will it still be snapshots of all profiling phases?

Or is the only valid value the current one? If so, why require the caller to
pass it as a param? Can't the impl track which one is the last one internally?

https://codereview.chromium.org/985773002/diff/1/content/browser/profiler_con...
File content/browser/profiler_controller_impl.cc (right):

https://codereview.chromium.org/985773002/diff/1/content/browser/profiler_con...
content/browser/profiler_controller_impl.cc:90: profiling_phase)))
Nit: {}'s

https://codereview.chromium.org/985773002/diff/1/content/public/browser/profi...
File content/public/browser/profiler_controller.h (right):

https://codereview.chromium.org/985773002/diff/1/content/public/browser/profi...
content/public/browser/profiler_controller.h:44: // Contact all processes and
get their profiler data.
Update comment.

[Ilya Sherman, 2015-03-06 20:59:42]

On 2015/03/06 20:06:40, Alexei Svitkine wrote:
> Still on the large side as CL goes, my preference would be to break it up into
> smaller chunks still.

Seconded.

[vadimt, 2015-03-06 21:26:44]

First comments

[vadimt, 2015-03-06 21:34:58]

Changes + rebase

I have no good idea (but open to suggestions) on how to split this. When you
change the data structure returned by base/, a lots of code consuming this
structure necessarily changes...

This CL consists of these changes:
1. New format for snapshots (monolithic => multi-phased), delivery it from base/
to components/ and using there.
2. Delivering phase change events from components/ to base/ and processing
there.

It doesn’t make sense to split either to smaller parts because they are pretty
much atomic. The biggest change is 1, but how can it be reasonably split if we
“1 to many”-transition a data structure used across many components?

I can in principle separate 1 from 2. But I think, this would make the review
harder. Imagine creating an object with only getter, and adding setter in the
next CL. To understand why getter works this way, you need setter.

However, if you insist, I can split 1 from 2, even though it will make reviewing
harder.

Feel free, though, to walk through *.h files differences and build your own
opinion.

https://codereview.chromium.org/985773002/diff/1/base/tracked_objects.cc
File base/tracked_objects.cc (right):

https://codereview.chromium.org/985773002/diff/1/base/tracked_objects.cc#newc...
base/tracked_objects.cc:417:
&completed_phases_snapshots_.Get()[profiling_phase]);
On 2015/03/06 20:06:40, Alexei Svitkine wrote:
> Can this function check that this isn't calling with the same profiling phase
> multiple times?

Done.

https://codereview.chromium.org/985773002/diff/1/base/tracked_objects.h
File base/tracked_objects.h (right):

https://codereview.chromium.org/985773002/diff/1/base/tracked_objects.h#newco...
base/tracked_objects.h:391: // phases, including the currect one, identified by
|current_profiling_phase|.
Correct. This must be the current profiler phase.
Imagine a renderer started after NonEmptyPaint event.
It doesn't know the last event, and cannot deduce whether it should return the
snapshot as belonging to phase 0 or 1.

https://codereview.chromium.org/985773002/diff/1/content/browser/profiler_con...
File content/browser/profiler_controller_impl.cc (right):

https://codereview.chromium.org/985773002/diff/1/content/browser/profiler_con...
content/browser/profiler_controller_impl.cc:90: profiling_phase)))
On 2015/03/06 20:06:40, Alexei Svitkine wrote:
> Nit: {}'s

Done.

https://codereview.chromium.org/985773002/diff/1/content/public/browser/profi...
File content/public/browser/profiler_controller.h (right):

https://codereview.chromium.org/985773002/diff/1/content/public/browser/profi...
content/public/browser/profiler_controller.h:44: // Contact all processes and
get their profiler data.
On 2015/03/06 20:06:40, Alexei Svitkine wrote:
> Update comment.

Done.

[vadimt, 2015-03-06 23:45:59]

isherman@ suggested a certain way of splitting, which I'm implementing now. Stay
tuned!

[vadimt, 2015-03-07 01:35:20]

Per a talk to isherman@:

1. base/ was minimally modified to return the new data format, but it always has
one phase. Most of the base/ changes are simply param types changes, but there
also are cleanups (moving loops to C++ 11 syntax and autoformatter changes)

2. The rest of the code now consumes the new data. However, there is no code
responsible for phase changes.

[vadimt, 2015-03-07 01:49:12]

Yeah, I mean this is already implemented, and please review.

[Alexei Svitkine (slow), 2015-03-09 17:13:36]

https://codereview.chromium.org/985773002/diff/40001/chrome/browser/task_prof...
File chrome/browser/task_profiler/task_profiler_data_serializer.cc (right):

https://codereview.chromium.org/985773002/diff/40001/chrome/browser/task_prof...
chrome/browser/task_profiler/task_profiler_data_serializer.cc:88: for (const
auto& i : process_data_phase.tasks) {
Nit: task

https://codereview.chromium.org/985773002/diff/40001/chrome/browser/task_prof...
chrome/browser/task_profiler/task_profiler_data_serializer.cc:100: for (const
auto& i : process_data_phase.descendants) {
Nit: i -> entry

https://codereview.chromium.org/985773002/diff/40001/components/metrics/profi...
File components/metrics/profiler/profiler_metrics_provider.cc (right):

https://codereview.chromium.org/985773002/diff/40001/components/metrics/profi...
components/metrics/profiler/profiler_metrics_provider.cc:83: for (const auto& i
: process_data_phase.tasks) {
Nit: |i| -> |task|

https://codereview.chromium.org/985773002/diff/40001/components/metrics/profi...
components/metrics/profiler/profiler_metrics_provider.cc:145: const
base::TimeDelta& profiling_phase_start,
Nit: If you name these two |phase_start| and |phase_finish| the code below I
think can avoid some wrapping. No need to prefix with "profiling_" since this is
all profiling related code.

https://codereview.chromium.org/985773002/diff/40001/components/metrics/profi...
components/metrics/profiler/profiler_metrics_provider.cc:157:
profiler_events_cache_.end();
Nit: ContainsKey() from stl_util.h.

https://codereview.chromium.org/985773002/diff/40001/components/metrics/profi...
File components/metrics/profiler/tracking_synchronizer.cc (right):

https://codereview.chromium.org/985773002/diff/40001/components/metrics/profi...
components/metrics/profiler/tracking_synchronizer.cc:259: // Send
process_data_snapshot from browser process.
Nit: surround in |'s or remove _'s.

https://codereview.chromium.org/985773002/diff/40001/components/metrics/profi...
components/metrics/profiler/tracking_synchronizer.cc:286: // comparison.
This logic seems sufficiently non trivial to warrant unit tests. Can you add
some?

https://codereview.chromium.org/985773002/diff/40001/components/metrics/profi...
File components/metrics/profiler/tracking_synchronizer.h (right):

https://codereview.chromium.org/985773002/diff/40001/components/metrics/profi...
components/metrics/profiler/tracking_synchronizer.h:116:
std::vector<ProfilerEventProto::ProfilerEvent>
Don't you have a typedef for this in another file? Or is the distinction that
this one is ordered but the other one isn't?

https://codereview.chromium.org/985773002/diff/40001/components/metrics/profi...
File components/metrics/profiler/tracking_synchronizer_observer.h (right):

https://codereview.chromium.org/985773002/diff/40001/components/metrics/profi...
components/metrics/profiler/tracking_synchronizer_observer.h:22: // Set of
profiling events, in no guaranteed order.
It's confusing to have something named a set when not backed by a set<>. I guess
it's probably because we don't need to have an efficient .find() on it so
vector<> is more efficient? If so, I'd mention that in the comment.

https://codereview.chromium.org/985773002/diff/40001/components/metrics/profi...
components/metrics/profiler/tracking_synchronizer_observer.h:36: const
tracked_objects::ProcessDataPhaseSnapshot& process_data_phase,
This is now a lot of parameters. How about defining a type to hold them and
passing that as a param?

https://codereview.chromium.org/985773002/diff/40001/components/metrics/profi...
components/metrics/profiler/tracking_synchronizer_observer.h:37: int process_id,
Is there a reason this is an int and not a base::ProcessId?

https://codereview.chromium.org/985773002/diff/40001/components/metrics/profi...
components/metrics/profiler/tracking_synchronizer_observer.h:38: int
process_type,
Again, shouldn't this be some enum type?

https://codereview.chromium.org/985773002/diff/40001/components/metrics/profi...
components/metrics/profiler/tracking_synchronizer_observer.h:39: int
profiling_phase,
Likewise - if this file is depending on the protobuf, why not use the enum from
there?

[Ilya Sherman, 2015-03-10 00:48:11]

Thanks, Vadim.  Please remember to update the CL description as well.

https://codereview.chromium.org/985773002/diff/40001/base/tracked_objects.cc
File base/tracked_objects.cc (right):

https://codereview.chromium.org/985773002/diff/40001/base/tracked_objects.cc#...
base/tracked_objects.cc:396:
&process_data_snapshot->phased_process_data_snapshots[0]);
What guarantees that the vector is non-empty?

https://codereview.chromium.org/985773002/diff/40001/base/tracked_objects.cc#...
base/tracked_objects.cc:591: void ThreadData::Snapshot(ProcessDataPhaseSnapshot*
process_data_phase) {
nit: I'd prefer to give this method a new name, and preserve the name of the
pre-existing method that was named "Snapshot" (i.e. the one that you've
currently renamed to "GetProcessDataSnapshot").

https://codereview.chromium.org/985773002/diff/40001/base/tracked_objects.cc#...
base/tracked_objects.cc:601: for (const auto& i : birth_counts) {
nit: Please use a more semantically contentful name for the variable, or at
least "it" for an iterator rather than "i".  My preference in this case would
probably be "birth" or "entry".

https://codereview.chromium.org/985773002/diff/40001/base/tracked_objects.cc#...
base/tracked_objects.cc:619: for (const auto& i : death_map) {
Same comment applies here, and for all other loops.

https://codereview.chromium.org/985773002/diff/40001/base/tracked_objects.cc#...
base/tracked_objects.cc:651: parent_child_set->insert(i);
I'd prefer to see the C++11-ization of the for loops in this file, and any other
purely stylistic cleanups", as a separate CL.  This is important for revision
history reasons -- it's misleading to see this line modified as part of a CL
that seems to be doing something major, when it's just an incidental style
change to slightly-related code.

https://codereview.chromium.org/985773002/diff/40001/base/tracked_objects.h
File base/tracked_objects.h (right):

https://codereview.chromium.org/985773002/diff/40001/base/tracked_objects.h#n...
base/tracked_objects.h:150: // identified by its 0-based nubmer.
nit: "0-based" -> "0-index"; "number" -> "id number" or "sequence number" or
something like that.

https://codereview.chromium.org/985773002/diff/40001/base/tracked_objects.h#n...
base/tracked_objects.h:352: typedef std::map<int, ProcessDataPhaseSnapshot>
PhasedProcessDataSnapshots;
As elsewhere, I'd prefer that you not use a typedef.  However, if you do use a
typedef, please include "Map" in the name.  By convention, a plural name means a
list.

Though, I wonder: Why do we need a map?  How can phases appear out of order?

https://codereview.chromium.org/985773002/diff/40001/base/tracked_objects.h#n...
base/tracked_objects.h:423: const TaskStopwatch& stopwatch);
nit: Are these two blocks just unrelated formatting changes?  Can they be split
off to a separate CL?

https://codereview.chromium.org/985773002/diff/40001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/profiler_ui.cc (right):

https://codereview.chromium.org/985773002/diff/40001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/profiler_ui.cc:176: process_data_phase, process_id,
process_type, &json_data);
Where are the corresponding changes to keep the chrome://profiler UI working as
it does today?

https://codereview.chromium.org/985773002/diff/40001/components/metrics/profi...
File components/metrics/profiler/tracking_synchronizer.cc (right):

https://codereview.chromium.org/985773002/diff/40001/components/metrics/profi...
components/metrics/profiler/tracking_synchronizer.cc:284: // Go though all
completed phases, and though the current one. The current
nit: "though" -> "through" (twice)

https://codereview.chromium.org/985773002/diff/40001/components/metrics/profi...
File components/metrics/profiler/tracking_synchronizer.h (right):

https://codereview.chromium.org/985773002/diff/40001/components/metrics/profi...
components/metrics/profiler/tracking_synchronizer.h:121: const base::TimeTicks
start_time_;
I think these two fields are only needed by the metrics service.  I'd prefer to
see them defined there.  That is, the TrackingSynchronizer should just pass
through data that it receives from //base, to any clients who register as
interested in the data.

This especially applies to the vector, which makes absolutely no sense outside
of the metrics service.  The TrackingSynchronizer should just pass the *current*
phase.  The metrics service can compute whatever clever cache it wants to from
there.

https://codereview.chromium.org/985773002/diff/40001/components/metrics/profi...
components/metrics/profiler/tracking_synchronizer.h:125:
std::vector<base::TimeTicks> phase_start_times_;
I'd prefer to see the time deltas computed in //base, rather than here.  I don't
think that it's accurate to assume that all processes have the same phase
durations.

https://codereview.chromium.org/985773002/diff/40001/components/metrics/profi...
File components/metrics/profiler/tracking_synchronizer_observer.h (right):

https://codereview.chromium.org/985773002/diff/40001/components/metrics/profi...
components/metrics/profiler/tracking_synchronizer_observer.h:22: // Set of
profiling events, in no guaranteed order.
On 2015/03/09 17:13:35, Alexei Svitkine wrote:
> It's confusing to have something named a set when not backed by a set<>. I
guess
> it's probably because we don't need to have an efficient .find() on it so
> vector<> is more efficient? If so, I'd mention that in the comment.

Or, just call this a "list".

https://codereview.chromium.org/985773002/diff/40001/components/metrics/profi...
components/metrics/profiler/tracking_synchronizer_observer.h:22: // Set of
profiling events, in no guaranteed order.
Why isn't the order guaranteed?

https://codereview.chromium.org/985773002/diff/40001/components/metrics/profi...
components/metrics/profiler/tracking_synchronizer_observer.h:23: typedef
std::vector<ProfilerEventProto::ProfilerEvent> ProfilerEventsSet;
Definitely please don't name the type "set" if it's backed by a vector.  I'd
prefer not to have a typedef at all... but if you must, the common convention is
to call this type "ProfilerEvents"

https://codereview.chromium.org/985773002/diff/40001/components/metrics/profi...
components/metrics/profiler/tracking_synchronizer_observer.h:32: // caused
finishing prior profiling phases.
nit: This final sentence is not very clear.  Please try to reword it so that
someone unfamiliar with this code could more easily understand what it means.

https://codereview.chromium.org/985773002/diff/40001/components/metrics/profi...
components/metrics/profiler/tracking_synchronizer_observer.h:36: const
tracked_objects::ProcessDataPhaseSnapshot& process_data_phase,
On 2015/03/09 17:13:35, Alexei Svitkine wrote:
> This is now a lot of parameters. How about defining a type to hold them and
> passing that as a param?

In particular, why aren't these all members of ProcessDataPhaseSnapshot?

https://codereview.chromium.org/985773002/diff/40001/content/child/child_thre...
File content/child/child_thread_impl.cc (right):

https://codereview.chromium.org/985773002/diff/40001/content/child/child_thre...
content/child/child_thread_impl.cc:646: process_data_snapshot));
Optional nit: IMO none of the renames here help with readability.  I don't feel
strongly about this, but I do think that shorter names, as long as they remain
clear, are generally better.

[vadimt, 2015-03-13 23:18:02]

https://codereview.chromium.org/985773002/diff/40001/base/tracked_objects.cc
File base/tracked_objects.cc (right):

https://codereview.chromium.org/985773002/diff/40001/base/tracked_objects.cc#...
base/tracked_objects.cc:396:
&process_data_snapshot->phased_process_data_snapshots[0]);
It's a map, and indexing it creates an entry.

https://codereview.chromium.org/985773002/diff/40001/base/tracked_objects.cc#...
base/tracked_objects.cc:591: void ThreadData::Snapshot(ProcessDataPhaseSnapshot*
process_data_phase) {
On 2015/03/10 00:48:10, Ilya Sherman wrote:
> nit: I'd prefer to give this method a new name, and preserve the name of the
> pre-existing method that was named "Snapshot" (i.e. the one that you've
> currently renamed to "GetProcessDataSnapshot").

Done.

https://codereview.chromium.org/985773002/diff/40001/base/tracked_objects.cc#...
base/tracked_objects.cc:601: for (const auto& i : birth_counts) {
On 2015/03/10 00:48:10, Ilya Sherman wrote:
> nit: Please use a more semantically contentful name for the variable, or at
> least "it" for an iterator rather than "i".  My preference in this case would
> probably be "birth" or "entry".

Done.

https://codereview.chromium.org/985773002/diff/40001/base/tracked_objects.cc#...
base/tracked_objects.cc:619: for (const auto& i : death_map) {
On 2015/03/10 00:48:10, Ilya Sherman wrote:
> Same comment applies here, and for all other loops.

Done.

https://codereview.chromium.org/985773002/diff/40001/base/tracked_objects.cc#...
base/tracked_objects.cc:651: parent_child_set->insert(i);
At some point, I've done C++11-ization by asvitkine request.
Now, it will take considerable time to separate it for a pretty small benefit.

I highly respect the time and convenience of reviewers, and the perfection of
the CL, but we should also, if possible, use the time of the reviewee
efficiently too. This way, we'll likely achieve maximal overall performance of
the review process.

Besides, there are no substantial changes in base/ in this CL, so we can
consider the base/ changes as mostly renaming and cleanup.

https://codereview.chromium.org/985773002/diff/40001/base/tracked_objects.h
File base/tracked_objects.h (right):

https://codereview.chromium.org/985773002/diff/40001/base/tracked_objects.h#n...
base/tracked_objects.h:150: // identified by its 0-based nubmer.
Done; the new terminology looks the most common.

https://codereview.chromium.org/985773002/diff/40001/base/tracked_objects.h#n...
base/tracked_objects.h:352: typedef std::map<int, ProcessDataPhaseSnapshot>
PhasedProcessDataSnapshots;
Done.
A process may start after several phase changes, i.e. phases will not start from
0.
Having this in mind, it's easiest to work with maps.

https://codereview.chromium.org/985773002/diff/40001/base/tracked_objects.h#n...
base/tracked_objects.h:423: const TaskStopwatch& stopwatch);
I'm using git cl format, which wants to format this...

I believe it's OK to have formatting changes on base/, since there are
practically no substantial changes there to be confused with.

https://codereview.chromium.org/985773002/diff/40001/chrome/browser/task_prof...
File chrome/browser/task_profiler/task_profiler_data_serializer.cc (right):

https://codereview.chromium.org/985773002/diff/40001/chrome/browser/task_prof...
chrome/browser/task_profiler/task_profiler_data_serializer.cc:88: for (const
auto& i : process_data_phase.tasks) {
On 2015/03/09 17:13:35, Alexei Svitkine wrote:
> Nit: task

Done.

https://codereview.chromium.org/985773002/diff/40001/chrome/browser/task_prof...
chrome/browser/task_profiler/task_profiler_data_serializer.cc:100: for (const
auto& i : process_data_phase.descendants) {
On 2015/03/09 17:13:35, Alexei Svitkine wrote:
> Nit: i -> entry

Done.

https://codereview.chromium.org/985773002/diff/40001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/profiler_ui.cc (right):

https://codereview.chromium.org/985773002/diff/40001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/profiler_ui.cc:176: process_data_phase, process_id,
process_type, &json_data);
JS code already does merging of whatever it receives. Note that we feed a single
phase at a time to it, like before.

https://codereview.chromium.org/985773002/diff/40001/components/metrics/profi...
File components/metrics/profiler/profiler_metrics_provider.cc (right):

https://codereview.chromium.org/985773002/diff/40001/components/metrics/profi...
components/metrics/profiler/profiler_metrics_provider.cc:145: const
base::TimeDelta& profiling_phase_start,
On 2015/03/09 17:13:35, Alexei Svitkine wrote:
> Nit: If you name these two |phase_start| and |phase_finish| the code below I
> think can avoid some wrapping. No need to prefix with "profiling_" since this
is
> all profiling related code.

Done.

https://codereview.chromium.org/985773002/diff/40001/components/metrics/profi...
components/metrics/profiler/profiler_metrics_provider.cc:157:
profiler_events_cache_.end();
On 2015/03/09 17:13:35, Alexei Svitkine wrote:
> Nit: ContainsKey() from stl_util.h.

Done.

https://codereview.chromium.org/985773002/diff/40001/components/metrics/profi...
File components/metrics/profiler/tracking_synchronizer.cc (right):

https://codereview.chromium.org/985773002/diff/40001/components/metrics/profi...
components/metrics/profiler/tracking_synchronizer.cc:259: // Send
process_data_snapshot from browser process.
On 2015/03/09 17:13:35, Alexei Svitkine wrote:
> Nit: surround in |'s or remove _'s.

Done.

https://codereview.chromium.org/985773002/diff/40001/components/metrics/profi...
components/metrics/profiler/tracking_synchronizer.cc:284: // Go though all
completed phases, and though the current one. The current
On 2015/03/10 00:48:10, Ilya Sherman wrote:
> nit: "though" -> "through" (twice)

Done.

https://codereview.chromium.org/985773002/diff/40001/components/metrics/profi...
components/metrics/profiler/tracking_synchronizer.cc:286: // comparison.
On 2015/03/09 17:13:35, Alexei Svitkine (slow) wrote:
> This logic seems sufficiently non trivial to warrant unit tests. Can you add
> some?

Done.

https://codereview.chromium.org/985773002/diff/40001/components/metrics/profi...
File components/metrics/profiler/tracking_synchronizer.h (right):

https://codereview.chromium.org/985773002/diff/40001/components/metrics/profi...
components/metrics/profiler/tracking_synchronizer.h:116:
std::vector<ProfilerEventProto::ProfilerEvent>
Yes. They are coincidentally same. I should be free to convert the other one
into a set, but this one will remain a vector.

https://codereview.chromium.org/985773002/diff/40001/components/metrics/profi...
components/metrics/profiler/tracking_synchronizer.h:121: const base::TimeTicks
start_time_;
Can we get back to this once you see the not-yet included methods? This CL is
incomplete in the sense that doesn't contain any code dealing with phase change
events.

https://codereview.chromium.org/985773002/diff/40001/components/metrics/profi...
components/metrics/profiler/tracking_synchronizer.h:125:
std::vector<base::TimeTicks> phase_start_times_;
We'll send one proto per phase. If each process provides its own version of the
time, it's not clear how to merge them.

https://codereview.chromium.org/985773002/diff/40001/components/metrics/profi...
File components/metrics/profiler/tracking_synchronizer_observer.h (right):

https://codereview.chromium.org/985773002/diff/40001/components/metrics/profi...
components/metrics/profiler/tracking_synchronizer_observer.h:22: // Set of
profiling events, in no guaranteed order.
On 2015/03/09 17:13:35, Alexei Svitkine wrote:
> It's confusing to have something named a set when not backed by a set<>. I
guess
> it's probably because we don't need to have an efficient .find() on it so
> vector<> is more efficient? If so, I'd mention that in the comment.

Done.

https://codereview.chromium.org/985773002/diff/40001/components/metrics/profi...
components/metrics/profiler/tracking_synchronizer_observer.h:22: // Set of
profiling events, in no guaranteed order.
The server doesn't use order. Hence, the client has no obligations to guarantee
the order. I'm adding this to comments so that no one later will feel obliges to
guarantee any order.

https://codereview.chromium.org/985773002/diff/40001/components/metrics/profi...
components/metrics/profiler/tracking_synchronizer_observer.h:23: typedef
std::vector<ProfilerEventProto::ProfilerEvent> ProfilerEventsSet;
On 2015/03/10 00:48:11, Ilya Sherman wrote:
> Definitely please don't name the type "set" if it's backed by a vector.  I'd
> prefer not to have a typedef at all... but if you must, the common convention
is
> to call this type "ProfilerEvents"

Done.

https://codereview.chromium.org/985773002/diff/40001/components/metrics/profi...
components/metrics/profiler/tracking_synchronizer_observer.h:32: // caused
finishing prior profiling phases.
On 2015/03/10 00:48:11, Ilya Sherman wrote:
> nit: This final sentence is not very clear.  Please try to reword it so that
> someone unfamiliar with this code could more easily understand what it means.

Done.

https://codereview.chromium.org/985773002/diff/40001/components/metrics/profi...
components/metrics/profiler/tracking_synchronizer_observer.h:36: const
tracked_objects::ProcessDataPhaseSnapshot& process_data_phase,
I don't feel very good about creating a copy of ProcessDataPhaseSnapshot just of
this... You think that this is worth the inefficiency?

https://codereview.chromium.org/985773002/diff/40001/components/metrics/profi...
components/metrics/profiler/tracking_synchronizer_observer.h:36: const
tracked_objects::ProcessDataPhaseSnapshot& process_data_phase,
Because they are calculated in TrackingSynchronizer, and
ProcessDataPhaseSnapshot is sent from base/. Also see my comment on why we don;t
send times from base/.

https://codereview.chromium.org/985773002/diff/40001/components/metrics/profi...
components/metrics/profiler/tracking_synchronizer_observer.h:37: int process_id,
On 2015/03/09 17:13:35, Alexei Svitkine wrote:
> Is there a reason this is an int and not a base::ProcessId?

Done.

https://codereview.chromium.org/985773002/diff/40001/components/metrics/profi...
components/metrics/profiler/tracking_synchronizer_observer.h:38: int
process_type,
On 2015/03/09 17:13:35, Alexei Svitkine wrote:
> Again, shouldn't this be some enum type?

Done, but I'd rather revert it since Nacl passes values outside of the range of
ProcessType enum, so I have to cast these values to ProcessType.

WDYT?

https://codereview.chromium.org/985773002/diff/40001/components/metrics/profi...
components/metrics/profiler/tracking_synchronizer_observer.h:39: int
profiling_phase,
No, this is just a number of the profiling phase. It's a consequential number.
For the protobuf-dependent value, see |past_profiler_events|.

https://codereview.chromium.org/985773002/diff/40001/content/child/child_thre...
File content/child/child_thread_impl.cc (right):

https://codereview.chromium.org/985773002/diff/40001/content/child/child_thre...
content/child/child_thread_impl.cc:646: process_data_snapshot));
Agreed. Must be a result of massive renames throughout the code.

[Ilya Sherman, 2015-03-14 00:02:31]

A few follow-up comments on the previous patch set.  Sending separately so that
they don't get buried and lost among the comments on the latest patch set.

https://codereview.chromium.org/985773002/diff/40001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/profiler_ui.cc (right):

https://codereview.chromium.org/985773002/diff/40001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/profiler_ui.cc:176: process_data_phase, process_id,
process_type, &json_data);
On 2015/03/13 23:18:01, vadimt wrote:
> JS code already does merging of whatever it receives. Note that we feed a
single
> phase at a time to it, like before.

Sorry, I don't understand what you mean.  Previously, there was no need to merge
data from a single process, since counters were never reset.  Now the API
expects that counters *can be* reset.  How is this API difference not
significant to the JS code?

https://codereview.chromium.org/985773002/diff/40001/components/metrics/profi...
File components/metrics/profiler/tracking_synchronizer.h (right):

https://codereview.chromium.org/985773002/diff/40001/components/metrics/profi...
components/metrics/profiler/tracking_synchronizer.h:121: const base::TimeTicks
start_time_;
On 2015/03/13 23:18:01, vadimt wrote:
> Can we get back to this once you see the not-yet included methods? This CL is
> incomplete in the sense that doesn't contain any code dealing with phase
change
> events.

Sure, we can come back to this.  Please mark it somehow in the latter CL, so
that we remember to.

https://codereview.chromium.org/985773002/diff/40001/components/metrics/profi...
components/metrics/profiler/tracking_synchronizer.h:125:
std::vector<base::TimeTicks> phase_start_times_;
On 2015/03/13 23:18:01, vadimt wrote:
> We'll send one proto per phase. If each process provides its own version of
the
> time, it's not clear how to merge them.

That suggests to me a flaw in our proto design, then.  The times are simply not
guaranteed to be aligned, so it's deceptive to send just a single pair of
timestamps for all the different processes.

https://codereview.chromium.org/985773002/diff/40001/components/metrics/profi...
File components/metrics/profiler/tracking_synchronizer_observer.h (right):

https://codereview.chromium.org/985773002/diff/40001/components/metrics/profi...
components/metrics/profiler/tracking_synchronizer_observer.h:36: const
tracked_objects::ProcessDataPhaseSnapshot& process_data_phase,
On 2015/03/13 23:18:02, vadimt wrote:
> I don't feel very good about creating a copy of ProcessDataPhaseSnapshot just
of
> this... You think that this is worth the inefficiency?

(a) I'd prefer to track all of this in //base, so that there's no need to create
copies.
(b) If we decide not to track all this in //base, I think the cost of a copy is
not very high.  We already create lots of copies of the data for serialization
across IPC and when bouncing the data across threads.  One more copy is not too
expensive.

But, if you are really concerned about the copy, *and* we don't track the data
in //base, we could still create a wrapper type that represents these
parameters, and references the ProcessDataPhaseSnapshot object as a
const-reference, to preserve efficiency.  I'd definitely want to see some
concrete data indicating that that level of optimization is useful before going
down that route, but it's an option.

[Ilya Sherman, 2015-03-14 00:20:35]

https://codereview.chromium.org/985773002/diff/40001/base/tracked_objects.h
File base/tracked_objects.h (right):

https://codereview.chromium.org/985773002/diff/40001/base/tracked_objects.h#n...
base/tracked_objects.h:352: typedef std::map<int, ProcessDataPhaseSnapshot>
PhasedProcessDataSnapshots;
On 2015/03/13 23:18:01, vadimt wrote:
> Done.
> A process may start after several phase changes, i.e. phases will not start
from
> 0.
> Having this in mind, it's easiest to work with maps.

I guess I was thinking that the phase number -- or even just the event that
triggered a phase change -- could be stored within the ProcessDataPhaseSnapshot
struct, since they're always passed together anyway.

https://codereview.chromium.org/985773002/diff/60001/components/metrics/profi...
File components/metrics/profiler/tracking_synchronizer.cc (right):

https://codereview.chromium.org/985773002/diff/60001/components/metrics/profi...
components/metrics/profiler/tracking_synchronizer.cc:183:
TrackingSynchronizer::TrackingSynchronizer(const base::TimeTicks& current_time)
nit: s/current_time/now

https://codereview.chromium.org/985773002/diff/60001/components/metrics/profi...
components/metrics/profiler/tracking_synchronizer.cc:183:
TrackingSynchronizer::TrackingSynchronizer(const base::TimeTicks& current_time)
Though, rather than passing the time, it might be more convenient to pass a
TickClock.

https://codereview.chromium.org/985773002/diff/60001/components/metrics/profi...
components/metrics/profiler/tracking_synchronizer.cc:316:
TrackingSynchronizerObserver* const observer =
nit: Is the const really needed here?  The pointer value is pretty clearly never
changed, so it just feels like visual noise -- especially since it might be
misread as synonymous with "const TrackingSynchronizerObserver*".

https://codereview.chromium.org/985773002/diff/60001/components/metrics/profi...
File components/metrics/profiler/tracking_synchronizer.h (right):

https://codereview.chromium.org/985773002/diff/60001/components/metrics/profi...
components/metrics/profiler/tracking_synchronizer.h:47:
TrackingSynchronizer(const base::TimeTicks& current_time);
nit: explicit

https://codereview.chromium.org/985773002/diff/60001/components/metrics/profi...
components/metrics/profiler/tracking_synchronizer.h:68: friend class
TrackingSynchronizerTest_ProfilerData_Test;
Please use protected methods rather than friending test code.  The advantage is
that the test API is clearer -- a friend can muck about with arbitrary private
state.

https://codereview.chromium.org/985773002/diff/60001/components/metrics/profi...
File components/metrics/profiler/tracking_synchronizer_observer.h (right):

https://codereview.chromium.org/985773002/diff/60001/components/metrics/profi...
components/metrics/profiler/tracking_synchronizer_observer.h:48: const
ProfilerEvents& past_events) = 0;
I don't think it's appropriate to pass a list (or set) of past events, as this
is only used by the metrics service client.  Please instead pass the event for
the phase itself.  The metrics service can manage its own cache, if need be. 
(In fact, since you're already passing the profiling_phase, there's probably no
need to pass the ProfilerEvent at all; or vice-versa.  One can be inferred from
the other.)

[vadimt, 2015-03-14 01:39:16]

https://codereview.chromium.org/985773002/diff/40001/base/tracked_objects.h
File base/tracked_objects.h (right):

https://codereview.chromium.org/985773002/diff/40001/base/tracked_objects.h#n...
base/tracked_objects.h:352: typedef std::map<int, ProcessDataPhaseSnapshot>
PhasedProcessDataSnapshots;
The event type is declared outside of base, and we can't refer it from base. We
can type-unsafely pass events converted to ints between base/ and consuming
code, but it's unreadable and error-prone.

Besides, for the current phase there is no event that closes it, so we'd have to
have to introduce a special value just for this case.

Besides besides, a process can start after some phases already completed, and if
we just include the event in this data structure, we'd not be able to to build
the set of prior events for each phase.

Hence, the current approach, where the consumer speaks with base/ in terms of
phase numbers, and centrally keeps track of the sequence of all events.

Speaking of the idea of storing the phase number inside
PhasedProcessDataSnapshot, I'm wonder why we'd need to store it inside while
it's already available as a key in the map. It's not quite aesthetically
attractive, is it?

https://codereview.chromium.org/985773002/diff/40001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/profiler_ui.cc (right):

https://codereview.chromium.org/985773002/diff/40001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/profiler_ui.cc:176: process_data_phase, process_id,
process_type, &json_data);
Maybe, it wasn't necessary, but I checked (i.e. fed test data and saw the result
on the tab) that JS code happens to merge data from same process as we need it.
I.e., you feed into it a sequence of phases from different processes, which can
contain 2 phases from exactly same process, and JS magically aggregates them.
Note that chrome://profiler doesn't show sampled values, do it doesn't need to
worry about merging them.

https://codereview.chromium.org/985773002/diff/40001/components/metrics/profi...
File components/metrics/profiler/tracking_synchronizer.h (right):

https://codereview.chromium.org/985773002/diff/40001/components/metrics/profi...
components/metrics/profiler/tracking_synchronizer.h:121: const base::TimeTicks
start_time_;
Added a TODO note that I'll remove in the next CL, hence causing a diff.

https://codereview.chromium.org/985773002/diff/40001/components/metrics/profi...
components/metrics/profiler/tracking_synchronizer.h:125:
std::vector<base::TimeTicks> phase_start_times_;
The alternative would be to send a proto per every phase in every process.
But it's semantically incorrect, since then profiling time, as seen from the
server, would multiply by the number of the processes.
What we are really interested at the server is the wallclock time gathered in
some central place, which is what we are doing. This reflects that fact that we
have a single user that experiences jank created by all processes running in
parallel, and the correct way to express jank frequency is in terms of user's
wallclock time.

https://codereview.chromium.org/985773002/diff/40001/components/metrics/profi...
File components/metrics/profiler/tracking_synchronizer_observer.h (right):

https://codereview.chromium.org/985773002/diff/40001/components/metrics/profi...
components/metrics/profiler/tracking_synchronizer_observer.h:36: const
tracked_objects::ProcessDataPhaseSnapshot& process_data_phase,
Can do, but first please confirm that you are OK with NOT sending times in
base/, per another comment.

I don;t recall if you suggested passing past_profiler_events from base as well,
but the objection would be that it's wrong to make base/ dependent on event
types defined outside of it...

https://codereview.chromium.org/985773002/diff/60001/components/metrics/profi...
File components/metrics/profiler/tracking_synchronizer.cc (right):

https://codereview.chromium.org/985773002/diff/60001/components/metrics/profi...
components/metrics/profiler/tracking_synchronizer.cc:183:
TrackingSynchronizer::TrackingSynchronizer(const base::TimeTicks& current_time)
On 2015/03/14 00:20:34, Ilya Sherman wrote:
> nit: s/current_time/now

Done.

https://codereview.chromium.org/985773002/diff/60001/components/metrics/profi...
components/metrics/profiler/tracking_synchronizer.cc:183:
TrackingSynchronizer::TrackingSynchronizer(const base::TimeTicks& current_time)
This would create more complexity in unit tests, and with caring about TickClock
lifetime, then create simplicity...

https://codereview.chromium.org/985773002/diff/60001/components/metrics/profi...
components/metrics/profiler/tracking_synchronizer.cc:316:
TrackingSynchronizerObserver* const observer =
Done. Unfortunately my style is not a dominating one...
I'd squeezed const in all places where it's only possible...

https://codereview.chromium.org/985773002/diff/60001/components/metrics/profi...
File components/metrics/profiler/tracking_synchronizer.h (right):

https://codereview.chromium.org/985773002/diff/60001/components/metrics/profi...
components/metrics/profiler/tracking_synchronizer.h:47:
TrackingSynchronizer(const base::TimeTicks& current_time);
On 2015/03/14 00:20:35, Ilya Sherman wrote:
> nit: explicit

Done.

https://codereview.chromium.org/985773002/diff/60001/components/metrics/profi...
components/metrics/profiler/tracking_synchronizer.h:68: friend class
TrackingSynchronizerTest_ProfilerData_Test;
Promising to do in the next CL.
The code is not complete, and I'd have to move some fields in the protected
section just for tests, and then not forget to move them back after a protected
methods that work with these fields are added.
BTW, I have a list of TODO items, wo, won't forget :)

https://codereview.chromium.org/985773002/diff/60001/components/metrics/profi...
File components/metrics/profiler/tracking_synchronizer_observer.h (right):

https://codereview.chromium.org/985773002/diff/60001/components/metrics/profi...
components/metrics/profiler/tracking_synchronizer_observer.h:48: const
ProfilerEvents& past_events) = 0;
This is very similar to the other theme that we decided to discuss at the later
CL.
Let's just speak about this later...

[Alexei Svitkine (slow), 2015-03-16 22:00:15]

https://codereview.chromium.org/985773002/diff/80001/base/tracked_objects.h
File base/tracked_objects.h (right):

https://codereview.chromium.org/985773002/diff/80001/base/tracked_objects.h#n...
base/tracked_objects.h:353: typedef std::map<int, ProcessDataPhaseSnapshot>
PhasedProcessDataSnapshotMap;
Nit: This shouldn't be in the middle of a bunch of forward decls. Move it below
them before ThreadData separated by blank lines.

https://codereview.chromium.org/985773002/diff/80001/chrome/browser/metrics/c...
File chrome/browser/metrics/chrome_metrics_service_client.cc (right):

https://codereview.chromium.org/985773002/diff/80001/chrome/browser/metrics/c...
chrome/browser/metrics/chrome_metrics_service_client.cc:387: const
metrics::ProfilerEvents& past_events) {
I still think we should group all of these fields on a struct or class, so that
code like this can just pass that object through, rather than re-writing these
params here and below.

https://codereview.chromium.org/985773002/diff/80001/components/metrics/profi...
File components/metrics/profiler/tracking_synchronizer.h (right):

https://codereview.chromium.org/985773002/diff/80001/components/metrics/profi...
components/metrics/profiler/tracking_synchronizer.h:92: // |process_type|.
Nit: Mention that |now| is a param for tests.

https://codereview.chromium.org/985773002/diff/80001/components/metrics/profi...
File components/metrics/profiler/tracking_synchronizer_unittest.cc (right):

https://codereview.chromium.org/985773002/diff/80001/components/metrics/profi...
components/metrics/profiler/tracking_synchronizer_unittest.cc:68: bool got0 =
false;
I'm not sure this syntax is allowed yet. Is it? If not, just init in the ctor.

Also, add a comment or rename to be more meaningful - e.g. got_phase_0,
got_phase_1.

https://codereview.chromium.org/985773002/diff/80001/components/metrics/profi...
components/metrics/profiler/tracking_synchronizer_unittest.cc:70: };
Nit: DISALLOW_COPY()

[vadimt, 2015-03-16 23:09:14]

https://codereview.chromium.org/985773002/diff/80001/base/tracked_objects.h
File base/tracked_objects.h (right):

https://codereview.chromium.org/985773002/diff/80001/base/tracked_objects.h#n...
base/tracked_objects.h:353: typedef std::map<int, ProcessDataPhaseSnapshot>
PhasedProcessDataSnapshotMap;
On 2015/03/16 22:00:15, Alexei Svitkine (slow) wrote:
> Nit: This shouldn't be in the middle of a bunch of forward decls. Move it
below
> them before ThreadData separated by blank lines.

Done.

https://codereview.chromium.org/985773002/diff/80001/chrome/browser/metrics/c...
File chrome/browser/metrics/chrome_metrics_service_client.cc (right):

https://codereview.chromium.org/985773002/diff/80001/chrome/browser/metrics/c...
chrome/browser/metrics/chrome_metrics_service_client.cc:387: const
metrics::ProfilerEvents& past_events) {
I suggest moving this to the next CL.
isherman@ questions the very set of these params, I object that their set will
become more clear in the next CL.
It will make more sense to consider grouping once we know which params this
function has.

https://codereview.chromium.org/985773002/diff/80001/components/metrics/profi...
File components/metrics/profiler/tracking_synchronizer.h (right):

https://codereview.chromium.org/985773002/diff/80001/components/metrics/profi...
components/metrics/profiler/tracking_synchronizer.h:92: // |process_type|.
On 2015/03/16 22:00:15, Alexei Svitkine (slow) wrote:
> Nit: Mention that |now| is a param for tests.

Done.

https://codereview.chromium.org/985773002/diff/80001/components/metrics/profi...
File components/metrics/profiler/tracking_synchronizer_unittest.cc (right):

https://codereview.chromium.org/985773002/diff/80001/components/metrics/profi...
components/metrics/profiler/tracking_synchronizer_unittest.cc:68: bool got0 =
false;
Worked for me.

https://codereview.chromium.org/985773002/diff/80001/components/metrics/profi...
components/metrics/profiler/tracking_synchronizer_unittest.cc:70: };
On 2015/03/16 22:00:15, Alexei Svitkine (slow) wrote:
> Nit: DISALLOW_COPY()

Done.

[Alexei Svitkine (slow), 2015-03-17 14:53:41]

https://codereview.chromium.org/985773002/diff/80001/chrome/browser/metrics/c...
File chrome/browser/metrics/chrome_metrics_service_client.cc (right):

https://codereview.chromium.org/985773002/diff/80001/chrome/browser/metrics/c...
chrome/browser/metrics/chrome_metrics_service_client.cc:387: const
metrics::ProfilerEvents& past_events) {
On 2015/03/16 23:09:13, vadimt wrote:
> I suggest moving this to the next CL.
> isherman@ questions the very set of these params, I object that their set will
> become more clear in the next CL.
> It will make more sense to consider grouping once we know which params this
> function has.

Can you elaborate what you're suggesting? i.e. are you suggesting to split this
CL so that this change will not be part of it or are you suggesting to land
these params as-is and discuss them in the next CL?

Obviously, the former is preferable, but I understand if its infeasible. If
you're suggesting the latter, please add a TODO and reference a crbug.

https://codereview.chromium.org/985773002/diff/80001/components/metrics/profi...
File components/metrics/profiler/tracking_synchronizer_unittest.cc (right):

https://codereview.chromium.org/985773002/diff/80001/components/metrics/profi...
components/metrics/profiler/tracking_synchronizer_unittest.cc:68: bool got0 =
false;
On 2015/03/16 23:09:14, vadimt wrote:
> Worked for me.

Sorry, I don't mean whether it compiles, but whether it's allowed by the style
guide. I think all C++11 features that have been allowed had a chromium-dev
thread about them. If this is indeed allowed, can you link to that thread? Or if
not, please switch to initializing in the ctor.

[vadimt, 2015-03-17 15:12:53]

https://codereview.chromium.org/985773002/diff/80001/chrome/browser/metrics/c...
File chrome/browser/metrics/chrome_metrics_service_client.cc (right):

https://codereview.chromium.org/985773002/diff/80001/chrome/browser/metrics/c...
chrome/browser/metrics/chrome_metrics_service_client.cc:387: const
metrics::ProfilerEvents& past_events) {
The latter.
Added TODO.

https://codereview.chromium.org/985773002/diff/80001/components/metrics/profi...
File components/metrics/profiler/tracking_synchronizer_unittest.cc (right):

https://codereview.chromium.org/985773002/diff/80001/components/metrics/profi...
components/metrics/profiler/tracking_synchronizer_unittest.cc:68: bool got0 =
false;
Not even allowed, but seems like encouraged:
"Use in-class member initialization for simple initializations, especially when
a member variable must be initialized the same way in more than one
constructor."

[Alexei Svitkine (slow), 2015-03-17 15:16:43]

lgtm, but please also wait for Ilya's review

https://codereview.chromium.org/985773002/diff/80001/components/metrics/profi...
File components/metrics/profiler/tracking_synchronizer_unittest.cc (right):

https://codereview.chromium.org/985773002/diff/80001/components/metrics/profi...
components/metrics/profiler/tracking_synchronizer_unittest.cc:68: bool got0 =
false;
On 2015/03/17 15:12:53, vadimt wrote:
> Not even allowed, but seems like encouraged:
> "Use in-class member initialization for simple initializations, especially
when
> a member variable must be initialized the same way in more than one
> constructor."

Great! Thanks for checking.

[Ilya Sherman, 2015-03-17 23:57:39]

https://codereview.chromium.org/985773002/diff/40001/base/tracked_objects.h
File base/tracked_objects.h (right):

https://codereview.chromium.org/985773002/diff/40001/base/tracked_objects.h#n...
base/tracked_objects.h:352: typedef std::map<int, ProcessDataPhaseSnapshot>
PhasedProcessDataSnapshots;
On 2015/03/14 01:39:15, vadimt wrote:
> The event type is declared outside of base, and we can't refer it from base.
We
> can type-unsafely pass events converted to ints between base/ and consuming
> code, but it's unreadable and error-prone.
> 
> Besides, for the current phase there is no event that closes it, so we'd have
to
> have to introduce a special value just for this case.
> 
> Besides besides, a process can start after some phases already completed, and
if
> we just include the event in this data structure, we'd not be able to to build
> the set of prior events for each phase.
> 
> Hence, the current approach, where the consumer speaks with base/ in terms of
> phase numbers, and centrally keeps track of the sequence of all events.

Fair enough.

> Speaking of the idea of storing the phase number inside
> PhasedProcessDataSnapshot, I'm wonder why we'd need to store it inside while
> it's already available as a key in the map. It's not quite aesthetically
> attractive, is it?

I'm suggesting not using a map, and instead just storing the phase number in the
snapshot.  I'm suggesting this because the two tend to be passed around
together, so they might as well be wrapped in the same struct.

https://codereview.chromium.org/985773002/diff/120001/base/tracked_objects_un...
File base/tracked_objects_unittest.cc (right):

https://codereview.chromium.org/985773002/diff/120001/base/tracked_objects_un...
base/tracked_objects_unittest.cc:74: ASSERT_EQ(1,
process_data.phased_process_data_snapshots.size());
nit: The "u" is important -- you're currently comparing a signed vs. unsigned
value.

https://codereview.chromium.org/985773002/diff/120001/base/tracked_objects_un...
base/tracked_objects_unittest.cc:75: const
PhasedProcessDataSnapshotMap::const_iterator it =
nit: auto

https://codereview.chromium.org/985773002/diff/120001/chrome/browser/task_pro...
File chrome/browser/task_profiler/task_profiler_data_serializer_unittest.cc
(right):

https://codereview.chromium.org/985773002/diff/120001/chrome/browser/task_pro...
chrome/browser/task_profiler/task_profiler_data_serializer_unittest.cc:21: }
Should this function be removed?

https://codereview.chromium.org/985773002/diff/120001/components/metrics/prof...
File components/metrics/profiler/profiler_metrics_provider.cc (right):

https://codereview.chromium.org/985773002/diff/120001/components/metrics/prof...
components/metrics/profiler/profiler_metrics_provider.cc:119: const
base::Callback<void(bool*)>& cellular_callback) {
Why did you drop the cellular_callback_ initialization code here?

https://codereview.chromium.org/985773002/diff/120001/components/metrics/prof...
components/metrics/profiler/profiler_metrics_provider.cc:133:
uma_proto->add_profiler_event()->CopyFrom(event.second);
Can you swap rather than copying?

https://codereview.chromium.org/985773002/diff/120001/components/metrics/prof...
components/metrics/profiler/profiler_metrics_provider.cc:155: const bool
new_phase = !ContainsKey(profiler_events_cache_, profiling_phase);
nit: I'd inline this code, rather than caching the result in the |new_phase|
variable.

https://codereview.chromium.org/985773002/diff/120001/components/metrics/prof...
components/metrics/profiler/profiler_metrics_provider.cc:165:
profiler_event->add_past_session_events(event);
This method name should be "add_past_session_event", which suggests that the
proto field is misnamed.

https://codereview.chromium.org/985773002/diff/120001/components/metrics/prof...
File components/metrics/profiler/tracking_synchronizer.cc (right):

https://codereview.chromium.org/985773002/diff/120001/components/metrics/prof...
components/metrics/profiler/tracking_synchronizer.cc:282:
tracked_objects::PhasedProcessDataSnapshotMap::const_iterator it =
nit: This is a good spot to use the "auto" type.

https://codereview.chromium.org/985773002/diff/120001/components/metrics/prof...
File components/metrics/profiler/tracking_synchronizer_observer.h (right):

https://codereview.chromium.org/985773002/diff/120001/components/metrics/prof...
components/metrics/profiler/tracking_synchronizer_observer.h:48: const
ProfilerEvents& past_events) = 0;
Besides the question of whether or not to wrap up all of these parameters into a
struct, I think that the last three parameters are very odd choices for this
API.  They all seem like things that should be implementation details of the
metrics provider, but those implementation details are seeping into this
interface instead.

In particular, I'd change the phase_start and phase_finish from TimeDeltas to
TimeTicks.  And I'd omit the |past_events| list -- either in favor of a single
ProfilerEvent that corresponds to the phase, or a method on the
TrackingSynchronizer that can translate a profiling_phase to a ProfilerEvent.

https://codereview.chromium.org/985773002/diff/120001/components/metrics/prof...
File components/metrics/profiler/tracking_synchronizer_unittest.cc (right):

https://codereview.chromium.org/985773002/diff/120001/components/metrics/prof...
components/metrics/profiler/tracking_synchronizer_unittest.cc:1: // Copyright
2014 The Chromium Authors. All rights reserved.
nit: 2015

https://codereview.chromium.org/985773002/diff/120001/components/metrics/prof...
components/metrics/profiler/tracking_synchronizer_unittest.cc:20: }
nit: Could you please "git cl format" this file if you haven't already?  I'm
pretty sure that the braces should be on the same line, i.e. "{}".

https://codereview.chromium.org/985773002/diff/120001/components/metrics/prof...
components/metrics/profiler/tracking_synchronizer_unittest.cc:36:
ASSERT_EQ(content::ProcessType::PROCESS_TYPE_PLUGIN, process_type);
nit: Can the ASSERT_*'s be EXPECT_*'s throughout this file, except in cases
where the code would actually crash if the expectation failed?

https://codereview.chromium.org/985773002/diff/120001/components/metrics/prof...
components/metrics/profiler/tracking_synchronizer_unittest.cc:79: }
nit: Please move the code above this line into an anonymous namespace.

https://codereview.chromium.org/985773002/diff/120001/components/metrics/prof...
components/metrics/profiler/tracking_synchronizer_unittest.cc:86:
scoped_refptr<TrackingSynchronizer> ts =
nit: Please use complete variable names, rather than abbreviations.

https://codereview.chromium.org/985773002/diff/120001/components/metrics/prof...
components/metrics/profiler/tracking_synchronizer_unittest.cc:107: TestObserver
to;
nit: Please use complete variable names, rather than abbreviations.

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

https://codereview.chromium.org/985773002/diff/40001/base/tracked_objects.h
File base/tracked_objects.h (right):

https://codereview.chromium.org/985773002/diff/40001/base/tracked_objects.h#n...
base/tracked_objects.h:352: typedef std::map<int, ProcessDataPhaseSnapshot>
PhasedProcessDataSnapshots;
> > Speaking of the idea of storing the phase number inside
> > PhasedProcessDataSnapshot, I'm wonder why we'd need to store it inside while
> > it's already available as a key in the map. It's not quite aesthetically
> > attractive, is it?
> 
> I'm suggesting not using a map, and instead just storing the phase number in
the
> snapshot.  I'm suggesting this because the two tend to be passed around
> together, so they might as well be wrapped in the same struct.

I'm using phase number to quickly find the entry in the map in
TrackingSynchronizer::SendData:
    tracked_objects::PhasedProcessDataSnapshotMap::const_iterator it =
        profiler_data.phased_process_data_snapshots.find(phase);

The could would look worse if I didn't have it as a key.

https://codereview.chromium.org/985773002/diff/120001/base/tracked_objects_un...
File base/tracked_objects_unittest.cc (right):

https://codereview.chromium.org/985773002/diff/120001/base/tracked_objects_un...
base/tracked_objects_unittest.cc:74: ASSERT_EQ(1,
process_data.phased_process_data_snapshots.size());
On 2015/03/17 23:57:38, Ilya Sherman wrote:
> nit: The "u" is important -- you're currently comparing a signed vs. unsigned
> value.

Done.

https://codereview.chromium.org/985773002/diff/120001/base/tracked_objects_un...
base/tracked_objects_unittest.cc:75: const
PhasedProcessDataSnapshotMap::const_iterator it =
On 2015/03/17 23:57:38, Ilya Sherman wrote:
> nit: auto

Done.

https://codereview.chromium.org/985773002/diff/120001/chrome/browser/task_pro...
File chrome/browser/task_profiler/task_profiler_data_serializer_unittest.cc
(right):

https://codereview.chromium.org/985773002/diff/120001/chrome/browser/task_pro...
chrome/browser/task_profiler/task_profiler_data_serializer_unittest.cc:21: }
On 2015/03/17 23:57:38, Ilya Sherman wrote:
> Should this function be removed?

Done.

https://codereview.chromium.org/985773002/diff/120001/components/metrics/prof...
File components/metrics/profiler/profiler_metrics_provider.cc (right):

https://codereview.chromium.org/985773002/diff/120001/components/metrics/prof...
components/metrics/profiler/profiler_metrics_provider.cc:119: const
base::Callback<void(bool*)>& cellular_callback) {
Merge :( Horrible :(

https://codereview.chromium.org/985773002/diff/120001/components/metrics/prof...
components/metrics/profiler/profiler_metrics_provider.cc:133:
uma_proto->add_profiler_event()->CopyFrom(event.second);
I can. However, why should I do this? Performance?

Note that I empty the whole profiler_events_cache_ map afterwards.

https://codereview.chromium.org/985773002/diff/120001/components/metrics/prof...
components/metrics/profiler/profiler_metrics_provider.cc:155: const bool
new_phase = !ContainsKey(profiler_events_cache_, profiling_phase);
The next line will unconditionally add the element to the map, and the same
expression will be always false. So, the semantics will change.

https://codereview.chromium.org/985773002/diff/120001/components/metrics/prof...
components/metrics/profiler/profiler_metrics_provider.cc:165:
profiler_event->add_past_session_events(event);
On 2015/03/17 23:57:38, Ilya Sherman wrote:
> This method name should be "add_past_session_event", which suggests that the
> proto field is misnamed.

Done.

https://codereview.chromium.org/985773002/diff/120001/components/metrics/prof...
File components/metrics/profiler/tracking_synchronizer.cc (right):

https://codereview.chromium.org/985773002/diff/120001/components/metrics/prof...
components/metrics/profiler/tracking_synchronizer.cc:282:
tracked_objects::PhasedProcessDataSnapshotMap::const_iterator it =
On 2015/03/17 23:57:38, Ilya Sherman wrote:
> nit: This is a good spot to use the "auto" type.

Done.

https://codereview.chromium.org/985773002/diff/120001/components/metrics/prof...
File components/metrics/profiler/tracking_synchronizer_observer.h (right):

https://codereview.chromium.org/985773002/diff/120001/components/metrics/prof...
components/metrics/profiler/tracking_synchronizer_observer.h:48: const
ProfilerEvents& past_events) = 0;
On 2015/03/17 23:57:38, Ilya Sherman wrote:
> Besides the question of whether or not to wrap up all of these parameters into
a
> struct, I think that the last three parameters are very odd choices for this
> API.  They all seem like things that should be implementation details of the
> metrics provider, but those implementation details are seeping into this
> interface instead.
> 
> In particular, I'd change the phase_start and phase_finish from TimeDeltas to
> TimeTicks.  And I'd omit the |past_events| list -- either in favor of a single
> ProfilerEvent that corresponds to the phase, or a method on the
> TrackingSynchronizer that can translate a profiling_phase to a ProfilerEvent. 


Al, these things fall into one of discussions postponed till the next CL this or
that way. Let's just get back to this later.

https://codereview.chromium.org/985773002/diff/120001/components/metrics/prof...
File components/metrics/profiler/tracking_synchronizer_unittest.cc (right):

https://codereview.chromium.org/985773002/diff/120001/components/metrics/prof...
components/metrics/profiler/tracking_synchronizer_unittest.cc:1: // Copyright
2014 The Chromium Authors. All rights reserved.
On 2015/03/17 23:57:39, Ilya Sherman wrote:
> nit: 2015

Done.

https://codereview.chromium.org/985773002/diff/120001/components/metrics/prof...
components/metrics/profiler/tracking_synchronizer_unittest.cc:20: }
I'm using it. Just it was OK. I placed {} on the same like, and it's OK again.

https://codereview.chromium.org/985773002/diff/120001/components/metrics/prof...
components/metrics/profiler/tracking_synchronizer_unittest.cc:79: }
On 2015/03/17 23:57:39, Ilya Sherman wrote:
> nit: Please move the code above this line into an anonymous namespace.

Done.

https://codereview.chromium.org/985773002/diff/120001/components/metrics/prof...
components/metrics/profiler/tracking_synchronizer_unittest.cc:86:
scoped_refptr<TrackingSynchronizer> ts =
On 2015/03/17 23:57:39, Ilya Sherman wrote:
> nit: Please use complete variable names, rather than abbreviations.

Done.

https://codereview.chromium.org/985773002/diff/120001/components/metrics/prof...
components/metrics/profiler/tracking_synchronizer_unittest.cc:107: TestObserver
to;
On 2015/03/17 23:57:38, Ilya Sherman wrote:
> nit: Please use complete variable names, rather than abbreviations.

Done.

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

LGTM with some topics for discussion punted to the next CL, plus some nits:

https://codereview.chromium.org/985773002/diff/40001/base/tracked_objects.h
File base/tracked_objects.h (right):

https://codereview.chromium.org/985773002/diff/40001/base/tracked_objects.h#n...
base/tracked_objects.h:352: typedef std::map<int, ProcessDataPhaseSnapshot>
PhasedProcessDataSnapshots;
On 2015/03/19 00:20:44, vadimt wrote:
> > > Speaking of the idea of storing the phase number inside
> > > PhasedProcessDataSnapshot, I'm wonder why we'd need to store it inside
while
> > > it's already available as a key in the map. It's not quite aesthetically
> > > attractive, is it?
> > 
> > I'm suggesting not using a map, and instead just storing the phase number in
> the
> > snapshot.  I'm suggesting this because the two tend to be passed around
> > together, so they might as well be wrapped in the same struct.
> 
> I'm using phase number to quickly find the entry in the map in
> TrackingSynchronizer::SendData:
>     tracked_objects::PhasedProcessDataSnapshotMap::const_iterator it =
>         profiler_data.phased_process_data_snapshots.find(phase);
> 
> The could would look worse if I didn't have it as a key.

I'm pretty unconvinced by this explanation -- it's pretty easy to call .find()
on a vector (which is also faster for small maps)... but I don't think it's
worth bikeshedding more on this, so I'll drop it.

https://codereview.chromium.org/985773002/diff/120001/components/metrics/prof...
File components/metrics/profiler/profiler_metrics_provider.cc (right):

https://codereview.chromium.org/985773002/diff/120001/components/metrics/prof...
components/metrics/profiler/profiler_metrics_provider.cc:133:
uma_proto->add_profiler_event()->CopyFrom(event.second);
On 2015/03/19 00:20:44, vadimt wrote:
> I can. However, why should I do this? Performance?
> 
> Note that I empty the whole profiler_events_cache_ map afterwards.

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.

https://codereview.chromium.org/985773002/diff/120001/components/metrics/prof...
File components/metrics/profiler/tracking_synchronizer_observer.h (right):

https://codereview.chromium.org/985773002/diff/120001/components/metrics/prof...
components/metrics/profiler/tracking_synchronizer_observer.h:48: const
ProfilerEvents& past_events) = 0;
On 2015/03/19 00:20:44, vadimt wrote:
> On 2015/03/17 23:57:38, Ilya Sherman wrote:
> > Besides the question of whether or not to wrap up all of these parameters
into
> a
> > struct, I think that the last three parameters are very odd choices for this
> > API.  They all seem like things that should be implementation details of the
> > metrics provider, but those implementation details are seeping into this
> > interface instead.
> > 
> > In particular, I'd change the phase_start and phase_finish from TimeDeltas
to
> > TimeTicks.  And I'd omit the |past_events| list -- either in favor of a
single
> > ProfilerEvent that corresponds to the phase, or a method on the
> > TrackingSynchronizer that can translate a profiling_phase to a
ProfilerEvent. 
> 
> 
> Al, these things fall into one of discussions postponed till the next CL this
or
> that way. Let's just get back to this later.

I don't think they're exactly related, but, um, okay.  Please add a TODO as a
reminder to come back to this.

https://codereview.chromium.org/985773002/diff/140001/base/tracked_objects_un...
File base/tracked_objects_unittest.cc (right):

https://codereview.chromium.org/985773002/diff/140001/base/tracked_objects_un...
base/tracked_objects_unittest.cc:74: ASSERT_EQ(1u,
process_data.phased_process_data_snapshots.size());
Please fix 1 -> "1u" throughout.

https://codereview.chromium.org/985773002/diff/140001/base/tracked_objects_un...
base/tracked_objects_unittest.cc:75: auto it =
process_data.phased_process_data_snapshots.find(0);
Please use "auto" throughout.

https://codereview.chromium.org/985773002/diff/140001/components/metrics/prof...
File components/metrics/profiler/tracking_synchronizer_unittest.cc (right):

https://codereview.chromium.org/985773002/diff/140001/components/metrics/prof...
components/metrics/profiler/tracking_synchronizer_unittest.cc:88: //
TrackingSynchronizerObserver.
nit: re-wrap?

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

https://codereview.chromium.org/985773002/diff/120001/components/metrics/prof...
File components/metrics/profiler/profiler_metrics_provider.cc (right):

https://codereview.chromium.org/985773002/diff/120001/components/metrics/prof...
components/metrics/profiler/profiler_metrics_provider.cc:133:
uma_proto->add_profiler_event()->CopyFrom(event.second);
On 2015/03/19 01:00:51, Ilya Sherman wrote:
> On 2015/03/19 00:20:44, vadimt wrote:
> > I can. However, why should I do this? Performance?
> > 
> > Note that I empty the whole profiler_events_cache_ map afterwards.
> 
> 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.

Done.

https://codereview.chromium.org/985773002/diff/120001/components/metrics/prof...
File components/metrics/profiler/tracking_synchronizer_observer.h (right):

https://codereview.chromium.org/985773002/diff/120001/components/metrics/prof...
components/metrics/profiler/tracking_synchronizer_observer.h:48: const
ProfilerEvents& past_events) = 0;
On 2015/03/19 01:00:51, Ilya Sherman wrote:
> On 2015/03/19 00:20:44, vadimt wrote:
> > On 2015/03/17 23:57:38, Ilya Sherman wrote:
> > > Besides the question of whether or not to wrap up all of these parameters
> into
> > a
> > > struct, I think that the last three parameters are very odd choices for
this
> > > API.  They all seem like things that should be implementation details of
the
> > > metrics provider, but those implementation details are seeping into this
> > > interface instead.
> > > 
> > > In particular, I'd change the phase_start and phase_finish from TimeDeltas
> to
> > > TimeTicks.  And I'd omit the |past_events| list -- either in favor of a
> single
> > > ProfilerEvent that corresponds to the phase, or a method on the
> > > TrackingSynchronizer that can translate a profiling_phase to a
> ProfilerEvent. 
> > 
> > 
> > Al, these things fall into one of discussions postponed till the next CL
this
> or
> > that way. Let's just get back to this later.
> 
> I don't think they're exactly related, but, um, okay.  Please add a TODO as a
> reminder to come back to this.

Done.

https://codereview.chromium.org/985773002/diff/140001/base/tracked_objects_un...
File base/tracked_objects_unittest.cc (right):

https://codereview.chromium.org/985773002/diff/140001/base/tracked_objects_un...
base/tracked_objects_unittest.cc:74: ASSERT_EQ(1u,
process_data.phased_process_data_snapshots.size());
On 2015/03/19 01:00:51, Ilya Sherman wrote:
> Please fix 1 -> "1u" throughout.

Done.

https://codereview.chromium.org/985773002/diff/140001/base/tracked_objects_un...
base/tracked_objects_unittest.cc:75: auto it =
process_data.phased_process_data_snapshots.find(0);
On 2015/03/19 01:00:51, Ilya Sherman wrote:
> Please use "auto" throughout.

Done.

https://codereview.chromium.org/985773002/diff/140001/components/metrics/prof...
File components/metrics/profiler/tracking_synchronizer_unittest.cc (right):

https://codereview.chromium.org/985773002/diff/140001/components/metrics/prof...
components/metrics/profiler/tracking_synchronizer_unittest.cc:88: //
TrackingSynchronizerObserver.
On 2015/03/19 01:00:51, Ilya Sherman wrote:
> nit: re-wrap?

Done.

[vadimt, 2015-03-19 18:12:42]

vadimt@chromium.org changed reviewers:
+ cpu@chromium.org

[vadimt, 2015-03-19 18:12:43]

cpu@chromium.org: Please provide OWNER's approval.
The whole set of files was thoroughly reviewed by pre-reviewed, who also own
some files.

FYI, the remaining files that they don't own (but still reviewed) are:
base/tracked_objects.h
base/tracked_objects.cc
base/tracked_objects_unittest.cc
chrome/browser/chrome_browser_main.cc
chrome/browser/task_profiler/task_profiler_data_serializer.h
chrome/browser/task_profiler/task_profiler_data_serializer.cc
chrome/browser/task_profiler/task_profiler_data_serializer_unittest.cc
chrome/browser/ui/webui/profiler_ui.h
chrome/browser/ui/webui/profiler_ui.cc
components/nacl/browser/nacl_broker_host_win.cc
components/nacl/browser/nacl_process_host.cc
content/browser/browser_child_process_host_impl.h
content/browser/browser_child_process_host_impl.cc
content/browser/profiler_controller_impl.h
content/browser/profiler_controller_impl.cc
content/browser/profiler_message_filter.h
content/browser/profiler_message_filter.cc
content/child/child_thread_impl.cc
content/common/child_process_messages.h
content/public/browser/browser_child_process_host.h
content/public/browser/profiler_subscriber.h

[cpu_(ooo_6.6-7.5), 2015-03-20 19:38:09]

can you please split the base change in a standalone CL?

I only took a quick peek, but are you only testing 1 phase?

[vadimt, 2015-03-20 20:19:03]

This CL is already a result of splitting the original CL into 5 parts :)
There are base/ changes that introduce a multi-phase data structure for profiler
data (+ admittedly, some cleanups). The rest of the code has changed to work
with new data (+ admittedly, some cleanups). 1 -> N transitions are never
simple.

Currently, only 1 phase is generated (phase 0), while more phases are expected
in the next CL (https://codereview.chromium.org/1021053003). Correspondingly,
yes, base/ tests check only 1 phase, and will test 2 phases in the next CL
(https://codereview.chromium.org/1021053003/diff/1/base/tracked_objects_unitte...).

Now, in principle, I can do a Cinderella job of splitting off all cleanups, and
other things, so that I get a minimal CL that includes base/ changes, while the
rest of the code is beaten to compile/work, but I wonder what I've done in my
previous life to deserve this :)

Perhaps, it will go better with the above explanations; however, let me know if
you really prefer me to separate base/ changes + bare minimal depending changes
into a separate CL.

[cpu_(ooo_6.6-7.5), 2015-03-26 20:31:43]

I am almost done. You still need security review for the IPC change.

https://codereview.chromium.org/985773002/diff/160001/content/browser/browser...
File content/browser/browser_child_process_host_impl.cc (right):

https://codereview.chromium.org/985773002/diff/160001/content/browser/browser...
content/browser/browser_child_process_host_impl.cc:63:
BrowserChildProcessHostDelegate* delegate) {
the changes here and in the .h file really out to be in a separate CL. I am
going to let it go this time since my delay but understand that if you send me a
simple CL it goes way quicker. 

When I see a 20 file CL it drops to the bottom of the queue until I have an hour
to spend on it. Then I realize 12 of the files have mechanical changes.

[vadimt, 2015-03-26 20:35:47]

https://codereview.chromium.org/985773002/diff/160001/content/browser/browser...
File content/browser/browser_child_process_host_impl.cc (right):

https://codereview.chromium.org/985773002/diff/160001/content/browser/browser...
content/browser/browser_child_process_host_impl.cc:63:
BrowserChildProcessHostDelegate* delegate) {
Ack

[vadimt, 2015-03-26 20:49:15]

vadimt@chromium.org changed reviewers:
+ dcheng@chromium.org

[vadimt, 2015-03-26 20:49:17]

dcheng@chromium.org: Please review changes in IPC messages.

Background: ChildProcessHostMsg_ChildProfilerData contains diagnostic
information for Chrome task executions. This is the information we show on
chrome://profiler tab, and also send in UMA.

What changed: Now we send essentially same information, but may split it to
"phases" (before, there was a single phase) to know, for example, which tasks
were executed before the browser was shown, and which - after.

[dcheng, 2015-03-27 02:50:45]

This is a pretty large patch. It's going to take me awhile to review.

[dcheng, 2015-03-27 08:36:38]

A few random things I noticed. I'm still tracing through the IPC bits, but these
comments are easy enough to address.

https://codereview.chromium.org/985773002/diff/160001/base/tracked_objects.cc
File base/tracked_objects.cc (right):

https://codereview.chromium.org/985773002/diff/160001/base/tracked_objects.cc...
base/tracked_objects.cc:645: for (auto& death : death_map_)
Slightly weird that 643 is const and this one is not.

https://codereview.chromium.org/985773002/diff/160001/chrome/browser/metrics/...
File chrome/browser/metrics/chrome_metrics_service_client.h (right):

https://codereview.chromium.org/985773002/diff/160001/chrome/browser/metrics/...
chrome/browser/metrics/chrome_metrics_service_client.h:101: const
base::TimeDelta& profiling_phase_start,
TimeDelta is typically passed by value:
https://code.google.com/p/chromium/codesearch#chromium/src/base/time/time.h&l=20

https://codereview.chromium.org/985773002/diff/160001/components/metrics/prof...
File components/metrics/profiler/tracking_synchronizer.h (right):

https://codereview.chromium.org/985773002/diff/160001/components/metrics/prof...
components/metrics/profiler/tracking_synchronizer.h:48: explicit
TrackingSynchronizer(const base::TimeTicks& now);
Similar comment about base::TimeTicks and passing by value.

[dcheng, 2015-03-27 09:32:53]

IPC changes LGTM, but please address the minor comments before landing.

https://codereview.chromium.org/985773002/diff/160001/components/metrics/prof...
File components/metrics/profiler/tracking_synchronizer.cc (right):

https://codereview.chromium.org/985773002/diff/160001/components/metrics/prof...
components/metrics/profiler/tracking_synchronizer.cc:298: if (phase !=
phase_completion_events_sequence_.size()) {
I think this would read a little more clearly as <

[vadimt, 2015-03-27 20:12:34]

https://codereview.chromium.org/985773002/diff/160001/base/tracked_objects.cc
File base/tracked_objects.cc (right):

https://codereview.chromium.org/985773002/diff/160001/base/tracked_objects.cc...
base/tracked_objects.cc:645: for (auto& death : death_map_)
On 2015/03/27 08:36:36, dcheng wrote:
> Slightly weird that 643 is const and this one is not.

Done.

https://codereview.chromium.org/985773002/diff/160001/components/metrics/prof...
File components/metrics/profiler/tracking_synchronizer.cc (right):

https://codereview.chromium.org/985773002/diff/160001/components/metrics/prof...
components/metrics/profiler/tracking_synchronizer.cc:298: if (phase !=
phase_completion_events_sequence_.size()) {
Done, but IMHO != is clearer. For example, for stl iterators we use !=.

https://codereview.chromium.org/985773002/diff/160001/components/metrics/prof...
File components/metrics/profiler/tracking_synchronizer.h (right):

https://codereview.chromium.org/985773002/diff/160001/components/metrics/prof...
components/metrics/profiler/tracking_synchronizer.h:48: explicit
TrackingSynchronizer(const base::TimeTicks& now);
On 2015/03/27 08:36:37, dcheng wrote:
> Similar comment about base::TimeTicks and passing by value.

Done.

[dcheng, 2015-03-30 06:56:06]

LGTM with nits.

https://codereview.chromium.org/985773002/diff/160001/components/metrics/prof...
File components/metrics/profiler/tracking_synchronizer.cc (right):

https://codereview.chromium.org/985773002/diff/160001/components/metrics/prof...
components/metrics/profiler/tracking_synchronizer.cc:298: if (phase !=
phase_completion_events_sequence_.size()) {
On 2015/03/27 20:12:34, vadimt wrote:
> Done, but IMHO != is clearer. For example, for stl iterators we use !=.

Perhaps if it's near the loop variable, but this is located further away.
Without nearby context, it's a bit more confusing if it's actually only matching
< or also matching something else--especially because this loop condition is <=,
which is a bit atypical as well.

https://codereview.chromium.org/985773002/diff/180001/components/metrics/prof...
File components/metrics/profiler/profiler_metrics_provider.h (right):

https://codereview.chromium.org/985773002/diff/180001/components/metrics/prof...
components/metrics/profiler/profiler_metrics_provider.h:41: const
base::TimeDelta& phase_start,
TimeDelta should be passed by value as well.

[vadimt, 2015-03-30 18:14:43]

https://codereview.chromium.org/985773002/diff/180001/components/metrics/prof...
File components/metrics/profiler/profiler_metrics_provider.h (right):

https://codereview.chromium.org/985773002/diff/180001/components/metrics/prof...
components/metrics/profiler/profiler_metrics_provider.h:41: const
base::TimeDelta& phase_start,
On 2015/03/30 06:56:06, dcheng wrote:
> TimeDelta should be passed by value as well.

Done.

[cpu_(ooo_6.6-7.5), 2015-03-30 20:04:11]

lgtm

[vadimt, 2015-03-30 20:11:19]

The CQ bit was checked by vadimt@chromium.org

[vadimt, 2015-03-30 20:11:20]

The patchset sent to the CQ was uploaded after l-g-t-m from
asvitkine@chromium.org, isherman@chromium.org, dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/985773002/#ps200001
(title: "More dcheng@ comments.")

[commit-bot: I haz the power, 2015-03-30 20:11:53]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/985773002/200001

[commit-bot: I haz the power, 2015-03-30 20:16:30]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-03-30 20:16:32]

Try jobs failed on following builders:
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)

[vadimt, 2015-03-30 21:29:01]

The CQ bit was checked by vadimt@chromium.org

[vadimt, 2015-03-30 21:29:02]

The patchset sent to the CQ was uploaded after l-g-t-m from
asvitkine@chromium.org, cpu@chromium.org, isherman@chromium.org,
dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/985773002/#ps220001
(title: "Rebasing.")

[commit-bot: I haz the power, 2015-03-30 21:29:47]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/985773002/220001

[commit-bot: I haz the power, 2015-03-30 21:41:43]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-03-30 21:41:45]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[vadimt, 2015-03-30 22:15:10]

The CQ bit was checked by vadimt@chromium.org

[vadimt, 2015-03-30 22:15:11]

The patchset sent to the CQ was uploaded after l-g-t-m from
asvitkine@chromium.org, cpu@chromium.org, isherman@chromium.org,
dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/985773002/#ps240001
(title: "Fixing DEPS file.")

[commit-bot: I haz the power, 2015-03-30 22:16:08]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/985773002/240001

[commit-bot: I haz the power, 2015-03-30 22:48:22]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-03-30 22:48:24]

Try jobs failed on following builders:
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)

[vadimt, 2015-03-30 23:13:23]

The CQ bit was checked by vadimt@chromium.org

[vadimt, 2015-03-30 23:13:24]

The patchset sent to the CQ was uploaded after l-g-t-m from
asvitkine@chromium.org, cpu@chromium.org, isherman@chromium.org,
dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/985773002/#ps260001
(title: "Fixing iOS compiler errors.")

[commit-bot: I haz the power, 2015-03-30 23:14:07]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/985773002/260001

[commit-bot: I haz the power, 2015-03-30 23:30:31]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-03-30 23:30:33]

Try jobs failed on following builders:
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)

[vadimt, 2015-03-31 00:57:30]

The CQ bit was checked by vadimt@chromium.org to run a CQ dry run

[vadimt, 2015-03-31 00:57:31]

The patchset sent to the CQ was uploaded after l-g-t-m from
asvitkine@chromium.org, cpu@chromium.org, isherman@chromium.org,
dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/985773002/#ps280001
(title: "Fixing more Mac/iOS compiler errors.")

[vadimt, 2015-03-31 00:57:59]

The CQ bit was unchecked by vadimt@chromium.org

[vadimt, 2015-03-31 00:58:15]

The CQ bit was checked by vadimt@chromium.org

[commit-bot: I haz the power, 2015-03-31 00:59:29]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/985773002/280001

[commit-bot: I haz the power, 2015-03-31 01:06:37]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-03-31 01:06:39]

Try jobs failed on following builders:
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)

[vadimt, 2015-03-31 15:36:09]

The CQ bit was checked by vadimt@chromium.org

[vadimt, 2015-03-31 15:36:10]

The patchset sent to the CQ was uploaded after l-g-t-m from
asvitkine@chromium.org, cpu@chromium.org, isherman@chromium.org,
dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/985773002/#ps320001
(title: "Fixing indents.")

[commit-bot: I haz the power, 2015-03-31 15:36:33]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/985773002/320001

[commit-bot: I haz the power, 2015-03-31 15:46:44]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-03-31 15:46:46]

Try jobs failed on following builders:
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)

[vadimt, 2015-03-31 19:58:42]

The CQ bit was checked by vadimt@chromium.org

[vadimt, 2015-03-31 19:58:43]

The patchset sent to the CQ was uploaded after l-g-t-m from
asvitkine@chromium.org, cpu@chromium.org, isherman@chromium.org,
dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/985773002/#ps340001
(title: "Temporary solution to compile on iOS, discussed with rohitrao@.")

[commit-bot: I haz the power, 2015-03-31 19:59:58]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/985773002/340001

[commit-bot: I haz the power, 2015-03-31 20:37:42]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-03-31 20:37:43]

Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)

[vadimt, 2015-03-31 21:30:39]

The CQ bit was checked by vadimt@chromium.org

[vadimt, 2015-03-31 21:30:40]

The patchset sent to the CQ was uploaded after l-g-t-m from
asvitkine@chromium.org, cpu@chromium.org, isherman@chromium.org,
dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/985773002/#ps360001
(title: "Fixing Android compile errors.")

[commit-bot: I haz the power, 2015-03-31 21:31:40]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/985773002/360001

[commit-bot: I haz the power, 2015-04-01 00:09:59]

Committed patchset #19 (id:360001)

[commit-bot: I haz the power, 2015-04-01 00:10:32]

Patchset 19 (id:??) landed as
https://crrev.com/379d7fe794b4a200e08ac3f23c69184edef085f7
Cr-Commit-Position: refs/heads/master@{#323151}