Dry run: Try jobs failed on following builders: Catapult Android Tryserver on master.tryserver.client.catapult (JOB_FAILED, https://build.chromium.org/p/tryserver.client.catapult/builders/Catapult%20Android%20Tryserver/builds/5273)
3 years, 4 months ago
(2017-09-15 23:44:58 UTC)
#4
3 years, 4 months ago
(2017-09-16 00:36:11 UTC)
#9
Dry run: This issue passed the CQ dry run.
ddeptford
This change utilizes the BattOr visualization with data from the Monsoon power monitor. The Monsoon ...
3 years, 4 months ago
(2017-09-18 17:47:51 UTC)
#10
This change utilizes the BattOr visualization with data from the Monsoon power
monitor. The Monsoon script used with data capture is from
/master/tools/utils/monsoon.py within Android (not the newer HVPM/LVPM toolset
from PyMonsoon).
Tracing in Monsoon mode disables USB while capture is in progress as not to
interfere with power measurement. When tracing is complete, the monsoon tracer
is stopped first which enables USB connectivity for the rest of the tracers to
stop.
Data capture latency error is limited by the start time of Monsoon capture, the
inaccuracy of monsoon.py's reporting (integer second accuracy) and local USB
latency dependent upon system configuration and physical USB connections. Since
the latter of these are locally dependent, an option is given to provide a
latency offset for monsoon data.
Chris Craik
Mostly looks good, but would like to hear from Zhen about usb stop/restart stuff https://codereview.chromium.org/3018533002/diff/20001/systrace/systrace/tracing_agents/monsoon_agent.py ...
3 years, 4 months ago
(2017-09-19 18:47:02 UTC)
#11
Mostly looks good, but would like to hear from Zhen about usb stop/restart stuff
https://codereview.chromium.org/3018533002/diff/20001/systrace/systrace/traci...
File systrace/systrace/tracing_agents/monsoon_agent.py (right):
https://codereview.chromium.org/3018533002/diff/20001/systrace/systrace/traci...
systrace/systrace/tracing_agents/monsoon_agent.py:134: return True
can you do the actual stop here? we'd like to avoid stop happening in
getResults, if possible. it makes it so all the stops, and all of the getresults
happen simultaneously
https://codereview.chromium.org/3018533002/diff/20001/systrace/systrace/traci...
File systrace/systrace/tracing_controller.py (right):
https://codereview.chromium.org/3018533002/diff/20001/systrace/systrace/traci...
systrace/systrace/tracing_controller.py:125: # If we have a monsoon power
monitor controller in the group, make an
I'm not especially happy with this, but I don't have other ideas. Zhen,
thoughts? May be nice to abstract this to a "connectionOwnerAgent" or something
like that, instead of hard coding this as a monsoon agent, then have only
monsoon return 'true' from an isConnectionOwner method.
Zhen, do we know if there are other agents that expect to use USB while tracing?
Alternately, we could consider another 'lifecycle' callback that happens after
all the starts, and before all the stops, and monsoon could put its code there.
Zhen Wang
Notice that Catapult is switching to Gerrit Code Review instead of Rietveld on 9/27. So ...
3 years, 4 months ago
(2017-09-19 23:53:50 UTC)
#12
Notice that Catapult is switching to Gerrit Code Review instead of Rietveld on
9/27. So maybe migrate this CL to Gerrit.
See:
https://groups.google.com/a/google.com/forum/#!topic/chrome-speed-operations/...https://codereview.chromium.org/3018533002/diff/20001/systrace/systrace/traci...
File systrace/systrace/tracing_agents/monsoon_agent.py (right):
https://codereview.chromium.org/3018533002/diff/20001/systrace/systrace/traci...
systrace/systrace/tracing_agents/monsoon_agent.py:134: return True
On 2017/09/19 18:47:02, Chris Craik wrote:
> can you do the actual stop here? we'd like to avoid stop happening in
> getResults, if possible. it makes it so all the stops, and all of the
getresults
> happen simultaneously
Seem that the actual stopping is done in StopCollectionWithClockSync, which is
specific to Monsoon only. And in current patch, it is explicitly called before
other agents are stopped.
This can be solved if we have a better way of controlling the USB connection.
https://codereview.chromium.org/3018533002/diff/20001/systrace/systrace/traci...
File systrace/systrace/tracing_controller.py (right):
https://codereview.chromium.org/3018533002/diff/20001/systrace/systrace/traci...
systrace/systrace/tracing_controller.py:125: # If we have a monsoon power
monitor controller in the group, make an
On 2017/09/19 18:47:02, Chris Craik wrote:
> I'm not especially happy with this, but I don't have other ideas. Zhen,
> thoughts? May be nice to abstract this to a "connectionOwnerAgent" or
something
> like that, instead of hard coding this as a monsoon agent, then have only
> monsoon return 'true' from an isConnectionOwner method.
>
> Zhen, do we know if there are other agents that expect to use USB while
tracing?
>
> Alternately, we could consider another 'lifecycle' callback that happens after
> all the starts, and before all the stops, and monsoon could put its code
there.
I see --usbpassthrough is set to off for monsoon. Looks like it also affects
other agents?
For systrace and adb_profile_chrome, I think agents do not need to use USB while
tracing. For Telemetry, it will need USB, but that has separate controlling
logic. So it should be fine.
Another lifecycle callback may not work well. It seems that the sequence is:
1. start tracing for other agents
2. disable usb
3. start tracing for monsoon
So it seems that monsoon's start cannot be grouped into step 1.
ConnectionOwnerAgent is probably the only way, so it will always start last and
stop first.
Chris Craik
On 2017/09/19 at 23:53:50, zhenw wrote: > Notice that Catapult is switching to Gerrit Code ...
3 years, 4 months ago
(2017-09-19 23:58:07 UTC)
#13
On 2017/09/19 at 23:53:50, zhenw wrote:
> Notice that Catapult is switching to Gerrit Code Review instead of Rietveld on
9/27. So maybe migrate this CL to Gerrit.
>
> See:
https://groups.google.com/a/google.com/forum/#!topic/chrome-speed-operations/...
>
>
https://codereview.chromium.org/3018533002/diff/20001/systrace/systrace/traci...
> File systrace/systrace/tracing_agents/monsoon_agent.py (right):
>
>
https://codereview.chromium.org/3018533002/diff/20001/systrace/systrace/traci...
> systrace/systrace/tracing_agents/monsoon_agent.py:134: return True
> On 2017/09/19 18:47:02, Chris Craik wrote:
> > can you do the actual stop here? we'd like to avoid stop happening in
> > getResults, if possible. it makes it so all the stops, and all of the
getresults
> > happen simultaneously
>
> Seem that the actual stopping is done in StopCollectionWithClockSync, which is
specific to Monsoon only. And in current patch, it is explicitly called before
other agents are stopped.
>
> This can be solved if we have a better way of controlling the USB connection.
>
>
https://codereview.chromium.org/3018533002/diff/20001/systrace/systrace/traci...
> File systrace/systrace/tracing_controller.py (right):
>
>
https://codereview.chromium.org/3018533002/diff/20001/systrace/systrace/traci...
> systrace/systrace/tracing_controller.py:125: # If we have a monsoon power
monitor controller in the group, make an
> On 2017/09/19 18:47:02, Chris Craik wrote:
> > I'm not especially happy with this, but I don't have other ideas. Zhen,
> > thoughts? May be nice to abstract this to a "connectionOwnerAgent" or
something
> > like that, instead of hard coding this as a monsoon agent, then have only
> > monsoon return 'true' from an isConnectionOwner method.
> >
> > Zhen, do we know if there are other agents that expect to use USB while
tracing?
> >
> > Alternately, we could consider another 'lifecycle' callback that happens
after
> > all the starts, and before all the stops, and monsoon could put its code
there.
>
> I see --usbpassthrough is set to off for monsoon. Looks like it also affects
other agents?
>
> For systrace and adb_profile_chrome, I think agents do not need to use USB
while tracing. For Telemetry, it will need USB, but that has separate
controlling logic. So it should be fine.
>
> Another lifecycle callback may not work well. It seems that the sequence is:
> 1. start tracing for other agents
> 2. disable usb
> 3. start tracing for monsoon
> So it seems that monsoon's start cannot be grouped into step 1.
>
> ConnectionOwnerAgent is probably the only way, so it will always start last
and stop first.
The other lifecycle methods I was imagining would be:
1 start tracing for all agents (monsoon ignores)
2 monsoon disables usb, and starts tracing (maybe call this 'startInnerTracing'
or something)
3 ...
4 monsoon ends tracing, reenables USB (maybe 'stopInnerTracing')
5 stop tracing callback for all agents (monsoon ignores)
Zhen Wang
> The other lifecycle methods I was imagining would be: > > 1 start tracing ...
3 years, 4 months ago
(2017-09-20 00:04:42 UTC)
#14
> The other lifecycle methods I was imagining would be:
>
> 1 start tracing for all agents (monsoon ignores)
> 2 monsoon disables usb, and starts tracing (maybe call this
'startInnerTracing'
> or something)
> 3 ...
> 4 monsoon ends tracing, reenables USB (maybe 'stopInnerTracing')
> 5 stop tracing callback for all agents (monsoon ignores)
I see. Probably more general as following:
1. StartTracingBeforeDisconnection
2. Disconnect
3. StartTracingAfterDisconnection
4 [tracing is running]
5. StopTracingBeforeReconnection
6. Reconnect
7. StopTracingAfterReconnection
Do we expect to have more than 1 agent that wants to disconnect before starting
tracing in the future? If not, it is probably simpler to just have something
like ConnectionOwnerAgent, because there is no API change.
Chris Craik
Sounds good, let's just go with the 'isConnectionOwnerAgent' bool method that monsoon agent can override, ...
3 years, 4 months ago
(2017-09-20 22:00:10 UTC)
#15
Sounds good, let's just go with the 'isConnectionOwnerAgent' bool method that
monsoon agent can override, and then have the tracing controller just operate on
that.
Daniel, if you want to go ahead and make that update to the CL here, we can
probably get it in before the gerrit switchover.
Or feel free to move to gerrit, whichever's fine.
ddeptford
https://codereview.chromium.org/3018533002/diff/20001/systrace/systrace/tracing_agents/monsoon_agent.py File systrace/systrace/tracing_agents/monsoon_agent.py (right): https://codereview.chromium.org/3018533002/diff/20001/systrace/systrace/tracing_agents/monsoon_agent.py#newcode134 systrace/systrace/tracing_agents/monsoon_agent.py:134: return True On 2017/09/19 18:47:02, Chris Craik wrote: > ...
3 years, 4 months ago
(2017-09-20 23:45:58 UTC)
#16
https://codereview.chromium.org/3018533002/diff/20001/systrace/systrace/traci...
File systrace/systrace/tracing_agents/monsoon_agent.py (right):
https://codereview.chromium.org/3018533002/diff/20001/systrace/systrace/traci...
systrace/systrace/tracing_agents/monsoon_agent.py:134: return True
On 2017/09/19 18:47:02, Chris Craik wrote:
> can you do the actual stop here? we'd like to avoid stop happening in
> getResults, if possible. it makes it so all the stops, and all of the
getresults
> happen simultaneously
Unfortunately this is a limitation of the monsoon script being in a subprocess -
it cannot re-enable USB while collecting power data from within the same
subprocess as sampling, and the sampling subprocess retains a lock on the serial
device. USB needs to be re-enabled prior to clock syncs being issued, which
happens prior to this method being called in the controller.
Hopefully this could change in the future if we integrate PyMonsoon (the
official Monsoon python library which also supports newer hardware).
ddeptford
The CQ bit was checked by ddeptford@google.com to run a CQ dry run
3 years, 4 months ago
(2017-09-21 01:22:07 UTC)
#17
Dry run: Try jobs failed on following builders: Catapult Android Tryserver on master.tryserver.client.catapult (JOB_FAILED, https://build.chromium.org/p/tryserver.client.catapult/builders/Catapult%20Android%20Tryserver/builds/5309)
3 years, 4 months ago
(2017-09-21 01:39:21 UTC)
#20
Try jobs failed on following builders: Catapult Presubmit on master.tryserver.client.catapult (JOB_FAILED, https://build.chromium.org/p/tryserver.client.catapult/builders/Catapult%20Presubmit/builds/9535)
3 years, 4 months ago
(2017-09-26 20:06:18 UTC)
#30
+simonhatch for common/ +charliea FYI Daniel, feel free to commit after Simon has taken a ...
3 years, 4 months ago
(2017-09-26 21:32:09 UTC)
#33
+simonhatch for common/
+charliea FYI
Daniel, feel free to commit after Simon has taken a look for common/. I will
take a look later once I get some time.
Zhen Wang
https://codereview.chromium.org/3018533002/diff/60001/systrace/systrace/tracing_agents/__init__.py File systrace/systrace/tracing_agents/__init__.py (right): https://codereview.chromium.org/3018533002/diff/60001/systrace/systrace/tracing_agents/__init__.py#newcode93 systrace/systrace/tracing_agents/__init__.py:93: # pylint: disable=no-self-use Why do we need this pylint ...
3 years, 4 months ago
(2017-09-28 01:58:49 UTC)
#34
Issue 3018533002: Implementing a Monsoon power monitor trace agent, utilizing the UI infrastructure that the BattOr a…
Created 3 years, 4 months ago by ddeptford
Modified 3 years, 4 months ago
Reviewers: Chris Craik, John Reck, Zhen Wang, Sami, shatch, charliea (OOO until 10-5)
Base URL:
Comments: 11