Makes the BattOrAgent asynchronous

Makes the BattOrAgent asynchronous

I confirmed with simonhatch@ that this made sense given the way that
the Chromium TracingController works. The agent will be communicating
over a serial connection, which probably shouldn't be done
synchronously.

BUG=542837
Committed: https://crrev.com/a36a5b07f1e6d68908f4f7c2f955c6ae434de7fa
Cr-Commit-Position: refs/heads/master@{#357144}

[charliea (OOO until 10-5), 2015-10-28 22:12:22]

Patchset #1 (id:1) has been deleted

[charliea (OOO until 10-5), 2015-10-28 22:15:59]

Patchset #1 (id:20001) has been deleted

[charliea (OOO until 10-5), 2015-10-29 17:06:36]

Patchset #1 (id:40001) has been deleted

[charliea (OOO until 10-5), 2015-10-29 17:09:50]

Description was changed from

==========
Makes the BattOrAgent asynchronous

BUG=542837
==========

to

==========
Makes the BattOrAgent asynchronous

BUG=542837
==========

[charliea (OOO until 10-5), 2015-10-29 17:11:11]

charliea@chromium.org changed reviewers:
+ zhenw@chromium.org

[charliea (OOO until 10-5), 2015-10-29 18:19:53]

Patchset #3 (id:100001) has been deleted

[charliea (OOO until 10-5), 2015-10-29 18:21:09]

As discussed offline, I went ahead and removed the callback from StartTracing(),
RecordClockSyncMarker(), and IssueClockSyncMarker() to match the existing
tracing agents. They'll now make a best-effort attempt to send a signal to the
Battor.

[Zhen Wang, 2015-10-29 19:40:21]

zhenw@chromium.org changed reviewers:
+ nednguyen@google.com

[Zhen Wang, 2015-10-29 19:40:22]

nednguyen, please take a look at the comment in battor_agent_bin.cc for the
discussion of using StopTracing() in an async way.

https://codereview.chromium.org/1407033008/diff/120001/tools/battor_agent/bat...
File tools/battor_agent/battor_agent.cc (right):

https://codereview.chromium.org/1407033008/diff/120001/tools/battor_agent/bat...
tools/battor_agent/battor_agent.cc:29: callback.Run(scoped_ptr<std::string>(new
std::string("battor trace output")));
I think this callback run eventually should be moved to somewhere else, right?
For example, after BattOr sends trace data back?

https://codereview.chromium.org/1407033008/diff/120001/tools/battor_agent/bat...
File tools/battor_agent/battor_agent.h (right):

https://codereview.chromium.org/1407033008/diff/120001/tools/battor_agent/bat...
tools/battor_agent/battor_agent.h:28: void RecordClockSyncMarker(const
std::string& marker);
I think we still need asynchronous call for this because we want to know when
this recording is done.

https://codereview.chromium.org/1407033008/diff/120001/tools/battor_agent/bat...
File tools/battor_agent/battor_agent_bin.cc (right):

https://codereview.chromium.org/1407033008/diff/120001/tools/battor_agent/bat...
tools/battor_agent/battor_agent_bin.cc:4: 
Can you also add some comments here to explain why we have battor_agent_bin and
who are the users of it?

https://codereview.chromium.org/1407033008/diff/120001/tools/battor_agent/bat...
tools/battor_agent/battor_agent_bin.cc:23: scoped_ptr<std::string>
g_trace_output;
Is this only for the purpose of printing it out for now? If we have clients for
this battor_agent_bin, do we still need this global variable?

If we want to use this binary by itself, maybe dump the result to a file instead
of using a global?

https://codereview.chromium.org/1407033008/diff/120001/tools/battor_agent/bat...
tools/battor_agent/battor_agent_bin.cc:75: g_stop_tracing_complete_event.Wait();
I am fine with treating this as a blocking call right now. But later when we
start to fill in the implementation details, we may need to rethink about this
from client's point of view. How will the client use this?

In Chrome, a trace data sink is used and a callback function is used to notify
when the trace data is available. In Telemetry, stop tracing is a synchronous
call. That's because it blocks on the Telemetry side to wait for the
notification of trace data being ready from the DevTools.

With additional tracing agents added, I think we need to use StopTracing in an
asynchronous way, so that all agents get stopped very quickly. And the the
controller waits for the agents' trace data to be ready.

+nednguyen for more thoughts on the Telemetry side.

[nednguyen, 2015-10-29 19:57:32]

https://codereview.chromium.org/1407033008/diff/120001/tools/battor_agent/bat...
File tools/battor_agent/battor_agent_bin.cc (right):

https://codereview.chromium.org/1407033008/diff/120001/tools/battor_agent/bat...
tools/battor_agent/battor_agent_bin.cc:23: scoped_ptr<std::string>
g_trace_output;
On 2015/10/29 19:40:22, Zhen Wang wrote:
> Is this only for the purpose of printing it out for now? If we have clients
for
> this battor_agent_bin, do we still need this global variable?
> 
> If we want to use this binary by itself, maybe dump the result to a file
instead
> of using a global?

Yeah, I am not a big fan of global. Can your StopTracing also take a string
*trace_output & later call the callback with it?

https://codereview.chromium.org/1407033008/diff/120001/tools/battor_agent/bat...
tools/battor_agent/battor_agent_bin.cc:75: g_stop_tracing_complete_event.Wait();
On 2015/10/29 19:40:22, Zhen Wang wrote:
> I am fine with treating this as a blocking call right now. But later when we
> start to fill in the implementation details, we may need to rethink about this
> from client's point of view. How will the client use this?
> 
> In Chrome, a trace data sink is used and a callback function is used to notify
> when the trace data is available. In Telemetry, stop tracing is a synchronous
> call. That's because it blocks on the Telemetry side to wait for the
> notification of trace data being ready from the DevTools.
> 
> With additional tracing agents added, I think we need to use StopTracing in an
> asynchronous way, so that all agents get stopped very quickly. And the the
> controller waits for the agents' trace data to be ready.

Sorry, but I feel like this is an unnecessary optimization. What gain do we get
from StopTracing on all agents asynchronously then have the tracing_controller
waiting for all data to be available?

> 
> +nednguyen for more thoughts on the Telemetry side.

[charliea (OOO until 10-5), 2015-10-29 21:12:12]

https://codereview.chromium.org/1407033008/diff/120001/tools/battor_agent/bat...
File tools/battor_agent/battor_agent.cc (right):

https://codereview.chromium.org/1407033008/diff/120001/tools/battor_agent/bat...
tools/battor_agent/battor_agent.cc:29: callback.Run(scoped_ptr<std::string>(new
std::string("battor trace output")));
On 2015/10/29 19:40:22, Zhen Wang wrote:
> I think this callback run eventually should be moved to somewhere else, right?
> For example, after BattOr sends trace data back?

Yea. 

This callback will likely be sent in as the callback for SerialIOHandler::Write
(https://code.google.com/p/chromium/codesearch#chromium/src/device/serial/seri...).

https://codereview.chromium.org/1407033008/diff/120001/tools/battor_agent/bat...
File tools/battor_agent/battor_agent_bin.cc (right):

https://codereview.chromium.org/1407033008/diff/120001/tools/battor_agent/bat...
tools/battor_agent/battor_agent_bin.cc:4: 
On 2015/10/29 19:40:22, Zhen Wang wrote:
> Can you also add some comments here to explain why we have battor_agent_bin
and
> who are the users of it?

Done.

https://codereview.chromium.org/1407033008/diff/120001/tools/battor_agent/bat...
tools/battor_agent/battor_agent_bin.cc:23: scoped_ptr<std::string>
g_trace_output;
Done. We wouldn't have needed the global variable when calling from Chromium
code, but I think this way is cleaner anyhow.

https://codereview.chromium.org/1407033008/diff/120001/tools/battor_agent/bat...
tools/battor_agent/battor_agent_bin.cc:75: g_stop_tracing_complete_event.Wait();
On 2015/10/29 19:57:32, nednguyen wrote:
> On 2015/10/29 19:40:22, Zhen Wang wrote:
> > I am fine with treating this as a blocking call right now. But later when we
> > start to fill in the implementation details, we may need to rethink about
this
> > from client's point of view. How will the client use this?
> > 
> > In Chrome, a trace data sink is used and a callback function is used to
notify
> > when the trace data is available. In Telemetry, stop tracing is a
synchronous
> > call. That's because it blocks on the Telemetry side to wait for the
> > notification of trace data being ready from the DevTools.
> > 
> > With additional tracing agents added, I think we need to use StopTracing in
an
> > asynchronous way, so that all agents get stopped very quickly. And the the
> > controller waits for the agents' trace data to be ready.
> 
> Sorry, but I feel like this is an unnecessary optimization. What gain do we
get
> from StopTracing on all agents asynchronously then have the tracing_controller
> waiting for all data to be available?
> 
> > 
> > +nednguyen for more thoughts on the Telemetry side.
> 

(I think we covered this in the meeting - we'll stick with this way for now, as
it's most consistent with how the surrounding code in Chrome works.)

[Zhen Wang, 2015-10-30 16:46:45]

lgtm with nits

https://codereview.chromium.org/1407033008/diff/160001/tools/battor_agent/bat...
File tools/battor_agent/battor_agent.cc (right):

https://codereview.chromium.org/1407033008/diff/160001/tools/battor_agent/bat...
tools/battor_agent/battor_agent.cc:15: // TODO: Open up a serial connection with
the BattOr.
TODO(charliea)

Same for other places.

https://codereview.chromium.org/1407033008/diff/160001/tools/battor_agent/bat...
File tools/battor_agent/battor_agent_bin.cc (right):

https://codereview.chromium.org/1407033008/diff/160001/tools/battor_agent/bat...
tools/battor_agent/battor_agent_bin.cc:28: scoped_ptr<std::string>
g_trace_output;
Remove this?

[charliea (OOO until 10-5), 2015-10-30 17:37:43]

https://codereview.chromium.org/1407033008/diff/160001/tools/battor_agent/bat...
File tools/battor_agent/battor_agent.cc (right):

https://codereview.chromium.org/1407033008/diff/160001/tools/battor_agent/bat...
tools/battor_agent/battor_agent.cc:15: // TODO: Open up a serial connection with
the BattOr.
On 2015/10/30 16:46:44, Zhen Wang wrote:
> TODO(charliea)
> 
> Same for other places.

Done.

https://codereview.chromium.org/1407033008/diff/160001/tools/battor_agent/bat...
File tools/battor_agent/battor_agent_bin.cc (right):

https://codereview.chromium.org/1407033008/diff/160001/tools/battor_agent/bat...
tools/battor_agent/battor_agent_bin.cc:28: scoped_ptr<std::string>
g_trace_output;
On 2015/10/30 16:46:44, Zhen Wang wrote:
> Remove this?

Ack. Sorry about that - not sure how that slipped back in.

[charliea (OOO until 10-5), 2015-10-30 17:37:55]

The CQ bit was checked by charliea@chromium.org

[charliea (OOO until 10-5), 2015-10-30 17:37:55]

The patchset sent to the CQ was uploaded after l-g-t-m from zhenw@chromium.org
Link to the patchset: https://codereview.chromium.org/1407033008/#ps180001
(title: "Code review")

[commit-bot: I haz the power, 2015-10-30 17:38:22]

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

[commit-bot: I haz the power, 2015-10-30 17:43:34]

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

[commit-bot: I haz the power, 2015-10-30 17:43:38]

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

[nednguyen(REVIEW IN OTHER ACC), 2015-10-30 17:52:13]

nednguyen@chromium.org changed reviewers:
+ nednguyen@chromium.org

[nednguyen(REVIEW IN OTHER ACC), 2015-10-30 17:52:13]

Lgtm

[charliea (OOO until 10-5), 2015-10-30 17:59:52]

The CQ bit was checked by charliea@chromium.org

[charliea (OOO until 10-5), 2015-10-30 17:59:52]

The patchset sent to the CQ was uploaded after l-g-t-m from zhenw@chromium.org,
nednguyen@chromium.org
Link to the patchset: https://codereview.chromium.org/1407033008/#ps200001
(title: "Added //base dependency to bin")

[commit-bot: I haz the power, 2015-10-30 18:00:19]

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

[charliea (OOO until 10-5), 2015-10-30 18:00:21]

Sorry about that Ned! I wasn't sure if you wanted me to wait for your lgtm or
not. I'm happy to wait on the next one, if you'd like

[nednguyen, 2015-10-30 18:26:16]

On 2015/10/30 18:00:21, charliea wrote:
> Sorry about that Ned! I wasn't sure if you wanted me to wait for your lgtm or
> not. I'm happy to wait on the next one, if you'd like

oh no problem, if Zhen is happy, you're good to go :-)

[commit-bot: I haz the power, 2015-10-30 18:40:15]

Committed patchset #7 (id:200001)

[commit-bot: I haz the power, 2015-10-30 18:41:16]

Patchset 7 (id:??) landed as
https://crrev.com/a36a5b07f1e6d68908f4f7c2f955c6ae434de7fa
Cr-Commit-Position: refs/heads/master@{#357144}