Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
25883003:
telemetry: Add Android Systrace profiler (Closed)
|
Created:
7 years, 2 months ago by Sami Modified:
7 years, 2 months ago CC:
chromium-reviews, chrome-speed-team+watch_google.com, telemetry+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
Descriptiontelemetry: Add Android Systrace profiler
Add support for capturing Android Systrace profiles in telemetry. The
resulting profile will contain events from both the Chrome trace as well
as systrace.
TEST=tools/perf/run_benchmark run --profiler=android-systrace --browser=android-chrome smoothness.top_25 --page-filter=facebook
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=229130
Patch Set 1 #
Total comments: 8
Patch Set 2 : Tony's comments. #
Total comments: 1
Patch Set 3 : Record a combined trace. #Patch Set 4 : Removed dupe print. #
Total comments: 1
Patch Set 5 : Address nits. #
Messages
Total messages: 13 (0 generated)
https://codereview.chromium.org/25883003/diff/1/tools/telemetry/telemetry/cor... File tools/telemetry/telemetry/core/platform/profiler/android_systrace_profiler.py (right): https://codereview.chromium.org/25883003/diff/1/tools/telemetry/telemetry/cor... tools/telemetry/telemetry/core/platform/profiler/android_systrace_profiler.py:13: 'gfx', Any way we can avoid the hardcoding? https://codereview.chromium.org/25883003/diff/1/tools/telemetry/telemetry/cor... tools/telemetry/telemetry/core/platform/profiler/android_systrace_profiler.py:42: subprocess.check_output( check_output is new in 2.7 and the bots still run 2.6. https://codereview.chromium.org/25883003/diff/1/tools/telemetry/telemetry/cor... tools/telemetry/telemetry/core/platform/profiler/android_systrace_profiler.py:43: ['adb', 'shell', 'atrace' , '--async_start'] + I think we want to use self._browser_backend.adb.Adb() instead of calling adb directly https://codereview.chromium.org/25883003/diff/1/tools/telemetry/telemetry/cor... tools/telemetry/telemetry/core/platform/profiler/android_systrace_profiler.py:98: print 'Wrote about://tracing systrace to', self._output_path The convention for profilers is to output instructions for opening the output. Copy from trace_profiler.py.
https://codereview.chromium.org/25883003/diff/1/tools/telemetry/telemetry/cor... File tools/telemetry/telemetry/core/platform/profiler/android_systrace_profiler.py (right): https://codereview.chromium.org/25883003/diff/1/tools/telemetry/telemetry/cor... tools/telemetry/telemetry/core/platform/profiler/android_systrace_profiler.py:13: 'gfx', On 2013/10/04 04:24:49, tonyg wrote: > Any way we can avoid the hardcoding? It doesn't look like there's a way to do that yet. I'll see if I can add something like --profiler-args to this patch. https://codereview.chromium.org/25883003/diff/1/tools/telemetry/telemetry/cor... tools/telemetry/telemetry/core/platform/profiler/android_systrace_profiler.py:42: subprocess.check_output( On 2013/10/04 04:24:49, tonyg wrote: > check_output is new in 2.7 and the bots still run 2.6. Ah, I didn't realize that. I'll fix this. Any plans to update the bots? Python 2.6 just turned 5 :) https://codereview.chromium.org/25883003/diff/1/tools/telemetry/telemetry/cor... tools/telemetry/telemetry/core/platform/profiler/android_systrace_profiler.py:43: ['adb', 'shell', 'atrace' , '--async_start'] + On 2013/10/04 04:24:49, tonyg wrote: > I think we want to use self._browser_backend.adb.Adb() instead of calling adb > directly This runs on a worker thread and it doesn't look like our adb wrapper is re-entrant[1]. [1] e.g., https://code.google.com/p/chromium/codesearch#chromium/src/third_party/androi... https://codereview.chromium.org/25883003/diff/1/tools/telemetry/telemetry/cor... tools/telemetry/telemetry/core/platform/profiler/android_systrace_profiler.py:98: print 'Wrote about://tracing systrace to', self._output_path On 2013/10/04 04:24:49, tonyg wrote: > The convention for profilers is to output instructions for opening the output. > Copy from trace_profiler.py. Done.
On Fri, Oct 4, 2013 at 6:41 AM, <skyostil@chromium.org> wrote: > > https://codereview.chromium.**org/25883003/diff/1/tools/** > telemetry/telemetry/core/**platform/profiler/android_** > systrace_profiler.py<https://codereview.chromium.org/25883003/diff/1/tools/telemetry/telemetry/core/platform/profiler/android_systrace_profiler.py> > File > tools/telemetry/telemetry/**core/platform/profiler/** > android_systrace_profiler.py > (right): > > https://codereview.chromium.**org/25883003/diff/1/tools/** > telemetry/telemetry/core/**platform/profiler/android_** > systrace_profiler.py#newcode13<https://codereview.chromium.org/25883003/diff/1/tools/telemetry/telemetry/core/platform/profiler/android_systrace_profiler.py#newcode13> > tools/telemetry/telemetry/**core/platform/profiler/** > android_systrace_profiler.py:**13: > 'gfx', > On 2013/10/04 04:24:49, tonyg wrote: > >> Any way we can avoid the hardcoding? >> > > It doesn't look like there's a way to do that yet. I'll see if I can add > something like --profiler-args to this patch. NP, you don't have to worry about that here. > > > https://codereview.chromium.**org/25883003/diff/1/tools/** > telemetry/telemetry/core/**platform/profiler/android_** > systrace_profiler.py#newcode42<https://codereview.chromium.org/25883003/diff/1/tools/telemetry/telemetry/core/platform/profiler/android_systrace_profiler.py#newcode42> > tools/telemetry/telemetry/**core/platform/profiler/** > android_systrace_profiler.py:**42: > subprocess.check_output( > On 2013/10/04 04:24:49, tonyg wrote: > >> check_output is new in 2.7 and the bots still run 2.6. >> > > Ah, I didn't realize that. I'll fix this. > > Any plans to update the bots? Python 2.6 just turned 5 :) Actually, you know what, 2.6 was only to support PyAuto which we no longer depend upon. I'll file a infra bug to try upgrading. > > > https://codereview.chromium.**org/25883003/diff/1/tools/** > telemetry/telemetry/core/**platform/profiler/android_** > systrace_profiler.py#newcode43<https://codereview.chromium.org/25883003/diff/1/tools/telemetry/telemetry/core/platform/profiler/android_systrace_profiler.py#newcode43> > tools/telemetry/telemetry/**core/platform/profiler/** > android_systrace_profiler.py:**43: > ['adb', 'shell', 'atrace' , '--async_start'] + > On 2013/10/04 04:24:49, tonyg wrote: > >> I think we want to use self._browser_backend.adb.Adb(**) instead of >> > calling adb > >> directly >> > > This runs on a worker thread and it doesn't look like our adb wrapper is > re-entrant[1]. > OK, maybe a comment? > > [1] e.g., > https://code.google.com/p/**chromium/codesearch#chromium/** > src/third_party/android_**testrunner/run_command.py&l=84<https://code.google.com/p/chromium/codesearch#chromium/src/third_party/android_testrunner/run_command.py&l=84> > > > https://codereview.chromium.**org/25883003/diff/1/tools/** > telemetry/telemetry/core/**platform/profiler/android_** > systrace_profiler.py#newcode98<https://codereview.chromium.org/25883003/diff/1/tools/telemetry/telemetry/core/platform/profiler/android_systrace_profiler.py#newcode98> > tools/telemetry/telemetry/**core/platform/profiler/** > android_systrace_profiler.py:**98: > print 'Wrote about://tracing systrace to', self._output_path > On 2013/10/04 04:24:49, tonyg wrote: > >> The convention for profilers is to output instructions for opening the >> > output. > >> Copy from trace_profiler.py. >> > > Done. > > https://codereview.chromium.**org/25883003/<https://codereview.chromium.org/2... > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/25883003/diff/6001/tools/telemetry/telemetry/... File tools/telemetry/telemetry/core/platform/profiler/android_systrace_profiler.py (right): https://codereview.chromium.org/25883003/diff/6001/tools/telemetry/telemetry/... tools/telemetry/telemetry/core/platform/profiler/android_systrace_profiler.py:61: self._trace_data = ''.join([zlib.decompress(d) for d in trace_data]) it's a bit of a shame to duplicate here with https://codereview.chromium.org/25686006/diff/8001/build/android/adb_profile_... can we haz one profilerz? :) maybe extend that "--continuous" there to do exactly this? i.e., call the adb_profile_chrome.py and block, then send a signal later on that is captured there and would persist the decoded traces?
On 2013/10/04 15:01:51, bulach wrote: > https://codereview.chromium.org/25883003/diff/6001/tools/telemetry/telemetry/... > File > tools/telemetry/telemetry/core/platform/profiler/android_systrace_profiler.py > (right): > > https://codereview.chromium.org/25883003/diff/6001/tools/telemetry/telemetry/... > tools/telemetry/telemetry/core/platform/profiler/android_systrace_profiler.py:61: > self._trace_data = ''.join([zlib.decompress(d) for d in trace_data]) > it's a bit of a shame to duplicate here with > https://codereview.chromium.org/25686006/diff/8001/build/android/adb_profile_... > > can we haz one profilerz? :) Great point! Nothing for this patch, but in the future, should we consider an interactive mode for Telemetry? Something that would allow you to load web pages and run profilers interactively instead of part of a benchmark? Then we could take advantage of coding up all of the profilers we know about in one place. > > maybe extend that "--continuous" there to do exactly this? > i.e., call the adb_profile_chrome.py and block, then send a signal later on that > is captured there and would persist the decoded traces?
On 2013/10/04 15:40:26, tonyg wrote: > On 2013/10/04 15:01:51, bulach wrote: > > > https://codereview.chromium.org/25883003/diff/6001/tools/telemetry/telemetry/... > > File > > tools/telemetry/telemetry/core/platform/profiler/android_systrace_profiler.py > > (right): > > > > > https://codereview.chromium.org/25883003/diff/6001/tools/telemetry/telemetry/... > > > tools/telemetry/telemetry/core/platform/profiler/android_systrace_profiler.py:61: > > self._trace_data = ''.join([zlib.decompress(d) for d in trace_data]) > > it's a bit of a shame to duplicate here with > > > https://codereview.chromium.org/25686006/diff/8001/build/android/adb_profile_... > > > > can we haz one profilerz? :) > > Great point! > > Nothing for this patch, but in the future, should we consider an interactive > mode for Telemetry? Something that would allow you to load web pages and run > profilers interactively instead of part of a benchmark? Then we could take > advantage of coding up all of the profilers we know about in one place. yeah, I already added something along these lines here: https://code.google.com/p/chromium/codesearch#chromium/src/tools/perf/record_... not sure if it covers all cases, but: ./tools/perf/record_android_profile.py --browser=android-chrome --profiler=foobar should do most of the "interactive" bits you mentioned.. :) I also added to the docs: http://www.chromium.org/developers/telemetry/profiling#TOC-Manual-Profiling--... I'd be happy to s/android// if you think it'd be useful for other platforms! :) ...as for "coding all the profilers": There are a few different levels and it'd vary per profiler, but in general: 1) in build/android/pylib is a good place for reusable components, and build/android for standalone / top-level scripts 2) alternatively, tools/android (such as memreport) 3) telemetry then wraps the ones above where feasible On this one specifically (and not necessarily for this patch), it'd be great to move all the logic to build/android, and have it here just a simple wrapper, like: https://code.google.com/p/chromium/codesearch#chromium/src/tools/telemetry/te... > > > > > maybe extend that "--continuous" there to do exactly this? > > i.e., call the adb_profile_chrome.py and block, then send a signal later on > that > > is captured there and would persist the decoded traces?
I've updated this to use adb_profile_chrome.py directly, which makes the patch much simpler than before. One complication is that because some telemetry benchmarks do tracing of their own, we need to use telemetry's own tracing backend to capture the Chrome trace events. We can't use adb_profile_chrome or systrace's webview category for this because it conflicts with telemetry's own tracing. So instead I save the traces from telemetry and systrace to a single .zip that can be opened in about://tracing directly. PTAL, thanks!
lgtm, neat, thanks!! https://codereview.chromium.org/25883003/diff/18001/tools/telemetry/telemetry... File tools/telemetry/telemetry/core/platform/profiler/android_systrace_profiler.py (right): https://codereview.chromium.org/25883003/diff/18001/tools/telemetry/telemetry... tools/telemetry/telemetry/core/platform/profiler/android_systrace_profiler.py:35: self._browser_backend.StartTracing(None, 10) nit: categories=None, timeout=10
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/skyostil@chromium.org/25883003/22001
Retried try job too often on mac_rel for step(s) interactive_ui_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/skyostil@chromium.org/25883003/22001
Message was sent while issue was closed.
Change committed as 229130 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
