[Tracing Clock Sync] Implement clock sync in Chrome

[Tracing Clock Sync] Implement clock sync in Chrome

This CL implements tracing clock sync in Chrome.
PowerTracingAgent is the first example.

Design doc: https://goo.gl/8Vy6qX

BUG=542390
Committed: https://crrev.com/556e3a531d1fccf63a46fb74971e4d6ad65fc4be
Cr-Commit-Position: refs/heads/master@{#366429}

[Zhen Wang, 2015-12-16 00:52:38]

Description was changed from

==========
[Tracing Clock Sync] Implement clock sync in Chrome

This CL implements tracing clock sync in Chrome.

BUG=
==========

to

==========
[Tracing Clock Sync] Implement clock sync in Chrome

This CL implements tracing clock sync in Chrome. 
PowerTracingAgent is the first example.

Design doc: https://goo.gl/8Vy6qX

BUG=542390
==========

[Zhen Wang, 2015-12-16 00:58:49]

zhenw@chromium.org changed reviewers:
+ oysteine@chromium.org, simonhatch@chromium.org

[Zhen Wang, 2015-12-16 00:58:49]

ptal

[oystein (OOO til 10th of July), 2015-12-16 02:10:07]

https://codereview.chromium.org/1526883005/diff/1/content/browser/tracing/pow...
File content/browser/tracing/power_tracing_agent.cc (right):

https://codereview.chromium.org/1526883005/diff/1/content/browser/tracing/pow...
content/browser/tracing/power_tracing_agent.cc:28: thread_.Start();
I'm not sure if the lifespan of the thread should be tied to the lifespan of the
singleton like this. Can we tie it to the lifespan of the tracing session
instead? I.e. start it when we start tracing, stop it when we end tracing.

https://codereview.chromium.org/1526883005/diff/1/content/browser/tracing/tra...
File content/browser/tracing/tracing_controller_impl.cc (right):

https://codereview.chromium.org/1526883005/diff/1/content/browser/tracing/tra...
content/browser/tracing/tracing_controller_impl.cc:942: return clock_sync_id_;
nit: return ++clock_sync_id_;

https://codereview.chromium.org/1526883005/diff/1/content/browser/tracing/tra...
content/browser/tracing/tracing_controller_impl.cc:948:
pending_clock_sync_ack_count_ = 0;
Can this be != 0 at this point? Would we ever have overlapping clock sync
requests? Should we DCHECK for this?

https://codereview.chromium.org/1526883005/diff/1/content/browser/tracing/tra...
content/browser/tracing/tracing_controller_impl.cc:949: for (auto it :
additional_tracing_agents_) {
nit: const auto& it

https://codereview.chromium.org/1526883005/diff/1/content/browser/tracing/tra...
content/browser/tracing/tracing_controller_impl.cc:972:
pending_clock_sync_ack_count_--;
nit: if(--pending_clock_sync_ack_count_ == 0).

https://codereview.chromium.org/1526883005/diff/1/content/browser/tracing/tra...
content/browser/tracing/tracing_controller_impl.cc:973: if
(pending_clock_sync_ack_count_ == 0)
I feel like this makes the API a little bit fragile; it'll break if the list of
agents changes or if one doesn't report back. Maybe we should add a timeout, at
least?

[Zhen Wang, 2015-12-16 23:49:12]

https://codereview.chromium.org/1526883005/diff/1/content/browser/tracing/pow...
File content/browser/tracing/power_tracing_agent.cc (right):

https://codereview.chromium.org/1526883005/diff/1/content/browser/tracing/pow...
content/browser/tracing/power_tracing_agent.cc:28: thread_.Start();
On 2015/12/16 02:10:05, Oystein wrote:
> I'm not sure if the lifespan of the thread should be tied to the lifespan of
the
> singleton like this. Can we tie it to the lifespan of the tracing session
> instead? I.e. start it when we start tracing, stop it when we end tracing.

If stopping the thread when we end tracing, thread_.Stop() will be called on the
UI thread. It will fail the check to allow IO. See the stack below:

#0 0x7f71320151be base::debug::StackTrace::StackTrace()
#1 0x7f713206f17f logging::LogMessage::~LogMessage()
#2 0x7f713216c76f base::ThreadRestrictions::AssertIOAllowed()
#3 0x7f713214f7d4 base::PlatformThread::Join()
#4 0x7f7132161fca base::Thread::Stop()
#5 0x7f7137fa9e8b content::PowerTracingAgent::OnStopTracingDone()

Is there a way to get around this? I see EtwSystemEventConsumer stops the thread
on the UI thread, why can it do that?
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/tr...

https://codereview.chromium.org/1526883005/diff/1/content/browser/tracing/tra...
File content/browser/tracing/tracing_controller_impl.cc (right):

https://codereview.chromium.org/1526883005/diff/1/content/browser/tracing/tra...
content/browser/tracing/tracing_controller_impl.cc:942: return clock_sync_id_;
On 2015/12/16 02:10:05, Oystein wrote:
> nit: return ++clock_sync_id_;

Done. Is there any reason to prefer combining those two statements into one?

https://codereview.chromium.org/1526883005/diff/1/content/browser/tracing/tra...
content/browser/tracing/tracing_controller_impl.cc:948:
pending_clock_sync_ack_count_ = 0;
On 2015/12/16 02:10:05, Oystein wrote:
> Can this be != 0 at this point? Would we ever have overlapping clock sync
> requests? Should we DCHECK for this?

Updated to CHECK. But how about the following scenario?
1. During the last tracing session, one agent disappeared (e.g., due to USB
disconnection for battor device).
2. So in the last tracing session, it timed out to stop tracing and
pending_clock_sync_ack_count_ never goes back to 0 because BattOrAgent never
reports back.
3. Now in the new tracing session, the DCHECK/CHECK will fail here.

https://codereview.chromium.org/1526883005/diff/1/content/browser/tracing/tra...
content/browser/tracing/tracing_controller_impl.cc:949: for (auto it :
additional_tracing_agents_) {
On 2015/12/16 02:10:05, Oystein wrote:
> nit: const auto& it

Done.

https://codereview.chromium.org/1526883005/diff/1/content/browser/tracing/tra...
content/browser/tracing/tracing_controller_impl.cc:972:
pending_clock_sync_ack_count_--;
On 2015/12/16 02:10:07, Oystein wrote:
> nit: if(--pending_clock_sync_ack_count_ == 0).

Done.

https://codereview.chromium.org/1526883005/diff/1/content/browser/tracing/tra...
content/browser/tracing/tracing_controller_impl.cc:973: if
(pending_clock_sync_ack_count_ == 0)
On 2015/12/16 02:10:05, Oystein wrote:
> I feel like this makes the API a little bit fragile; it'll break if the list
of
> agents changes or if one doesn't report back. Maybe we should add a timeout,
at
> least?

Done.

[Zhen Wang, 2015-12-18 00:12:09]

Oystein, I have updated the CL according to our discussion offline. Can you take
another look?

[oystein (OOO til 10th of July), 2015-12-21 16:54:42]

Great! lgtm from my side, not sure if simon has any more feedback.

[nednguyen, 2015-12-21 17:04:14]

nednguyen@google.com changed reviewers:
+ nednguyen@google.com

[nednguyen, 2015-12-21 17:04:15]

https://codereview.chromium.org/1526883005/diff/40001/content/browser/tracing...
File content/browser/tracing/battor_power_trace_provider.h (right):

https://codereview.chromium.org/1526883005/diff/40001/content/browser/tracing...
content/browser/tracing/battor_power_trace_provider.h:4: 
Can we remove this file? I think it's replaced by power_tracing_agent?

[Zhen Wang, 2015-12-21 17:10:21]

https://codereview.chromium.org/1526883005/diff/40001/content/browser/tracing...
File content/browser/tracing/battor_power_trace_provider.h (right):

https://codereview.chromium.org/1526883005/diff/40001/content/browser/tracing...
content/browser/tracing/battor_power_trace_provider.h:4: 
On 2015/12/21 17:04:15, nednguyen wrote:
> Can we remove this file? I think it's replaced by power_tracing_agent?

This will be replaced by battor_agent.h. Since it is not ready yet, I think it
is good to have follow-up CLs to hook it up and then remove this file.

[shatch, 2015-12-21 17:10:24]

lgtm

https://codereview.chromium.org/1526883005/diff/40001/content/browser/tracing...
File content/browser/tracing/battor_power_trace_provider.h (right):

https://codereview.chromium.org/1526883005/diff/40001/content/browser/tracing...
content/browser/tracing/battor_power_trace_provider.h:4: 
On 2015/12/21 17:04:15, nednguyen wrote:
> Can we remove this file? I think it's replaced by power_tracing_agent?

I'm fine with keeping this to house all the internals of communicating with
BattOr, feels cleaner than including it in the power_tracing_agent.

[nednguyen, 2015-12-21 17:23:39]

ok, lgtm

[Zhen Wang, 2015-12-21 17:31:40]

The CQ bit was checked by zhenw@chromium.org

[commit-bot: I haz the power, 2015-12-21 17:32:33]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1526883005/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1526883005/40001

[commit-bot: I haz the power, 2015-12-21 18:49:04]

Description was changed from

==========
[Tracing Clock Sync] Implement clock sync in Chrome

This CL implements tracing clock sync in Chrome. 
PowerTracingAgent is the first example.

Design doc: https://goo.gl/8Vy6qX

BUG=542390
==========

to

==========
[Tracing Clock Sync] Implement clock sync in Chrome

This CL implements tracing clock sync in Chrome. 
PowerTracingAgent is the first example.

Design doc: https://goo.gl/8Vy6qX

BUG=542390
==========

[commit-bot: I haz the power, 2015-12-21 18:49:07]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-12-21 18:50:48]

Description was changed from

==========
[Tracing Clock Sync] Implement clock sync in Chrome

This CL implements tracing clock sync in Chrome. 
PowerTracingAgent is the first example.

Design doc: https://goo.gl/8Vy6qX

BUG=542390
==========

to

==========
[Tracing Clock Sync] Implement clock sync in Chrome

This CL implements tracing clock sync in Chrome.
PowerTracingAgent is the first example.

Design doc: https://goo.gl/8Vy6qX

BUG=542390

Committed: https://crrev.com/556e3a531d1fccf63a46fb74971e4d6ad65fc4be
Cr-Commit-Position: refs/heads/master@{#366429}
==========

[commit-bot: I haz the power, 2015-12-21 18:50:49]

Patchset 3 (id:??) landed as
https://crrev.com/556e3a531d1fccf63a46fb74971e4d6ad65fc4be
Cr-Commit-Position: refs/heads/master@{#366429}