Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2712163002:
[Systrace] Fix systrace clock syncing issue with BattOr. (Closed)
|
Created:
3 years, 10 months ago by rnephew (Reviews Here) Modified:
3 years, 8 months ago CC:
catapult-reviews_chromium.org, telemetry-reviews_chromium.org Target Ref:
refs/heads/master Project:
catapult Visibility:
Public. |
Description[Systrace] Fix systrace clock syncing issue with BattOr.
It was broken before and they were aligned several hours apart.
By explicitly specifying the clock domain in the trace metadata (by using the same output formatter as Telemetry), the trace importer uses that clock domain for the systrace controller trace rather than the default (UNKNOWN_CHROME_LEGACY). This means that it avoids collapsing the systrace controller trace (from the host) and
LINUX_CLOCK_MONOTONIC (on the device) into a single clock domain. Since the clocks are on different devices, this would result in misaligned traces.
BUG=catapult:#3262
Review-Url: https://codereview.chromium.org/2712163002
Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapult/+/790242054d40097c6af06bf42804f73e998eb60c
Patch Set 1 #
Total comments: 2
Patch Set 2 : branch approach, old way if only atrace new if multitrace #
Total comments: 1
Patch Set 3 : fix tests #
Total comments: 10
Patch Set 4 : [Systrace] Fix systrace so that it works. #
Total comments: 12
Patch Set 5 : charlies comments #Patch Set 6 : rebase https://codereview.chromium.org/2736123003/ #Patch Set 7 : no more telemetry references #
Total comments: 21
Patch Set 8 : [Systrace] Fix systrace clock syncing issue with BattOr. #
Total comments: 4
Patch Set 9 : rebase and change todo #
Total comments: 1
Messages
Total messages: 70 (40 generated)
rnephew@chromium.org changed reviewers: + ccraik@google.com, charliea@chromium.org, nednguyen@google.com
https://codereview.chromium.org/2712163002/diff/1/systrace/profile_chrome/chr... File systrace/profile_chrome/chrome_startup_tracing_agent.py (right): https://codereview.chromium.org/2712163002/diff/1/systrace/profile_chrome/chr... systrace/profile_chrome/chrome_startup_tracing_agent.py:15: from telemetry.timeline import trace_data as trace_data_module I eventually would like to move this out of telemetry into either its own part of the repo so that it doesn't rely on telemetry. https://codereview.chromium.org/2712163002/diff/1/systrace/profile_chrome/chr... systrace/profile_chrome/chrome_startup_tracing_agent.py:80: def CollectAgentTraceData(self, trace_data_builder): Made changes to make it operate more like the tracing in telemetry.
The CQ bit was checked by rnephew@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Why are you removing the calls to output_generator? What made that approach start to fail?
On 2017/02/24 18:19:23, Chris Craik wrote: > Why are you removing the calls to output_generator? What made that approach > start to fail? It was no longer properly detecting clock syncs using output_generator. I am moving it to better match how telemetry does it, so that we do not have two diverging paths to maintain.
On 2017/02/24 at 18:21:42, rnephew wrote: > On 2017/02/24 18:19:23, Chris Craik wrote: > > Why are you removing the calls to output_generator? What made that approach > > start to fail? > > It was no longer properly detecting clock syncs using output_generator. I am moving it to better match how telemetry does it, so that we do not have two diverging paths to maintain. Unfortunately, we need to continue using output_generator's approach for the time being. The ui-based Android SDK tool used to generate systraces uses the same trace construction approach, reimplemented in java, to generate trace files for developers: https://developer.android.com/studio/profile/systrace-walkthru.html shows the capture UI (and mistakenly says python is required) Because the java implementation isn't maintained to keep in sync with the python version, keeping our python approach stable and continuing to use it makes breaking the java version (when changes occur in the viewer code) much less likely.
On 2017/02/24 18:27:30, Chris Craik wrote: > On 2017/02/24 at 18:21:42, rnephew wrote: > > On 2017/02/24 18:19:23, Chris Craik wrote: > > > Why are you removing the calls to output_generator? What made that approach > > > start to fail? > > > > It was no longer properly detecting clock syncs using output_generator. I am > moving it to better match how telemetry does it, so that we do not have two > diverging paths to maintain. > > Unfortunately, we need to continue using output_generator's approach for the > time being. The ui-based Android SDK tool used to generate systraces uses the > same trace construction approach, reimplemented in java, to generate trace files > for developers: > https://developer.android.com/studio/profile/systrace-walkthru.html shows the > capture UI (and mistakenly says python is required) > > Because the java implementation isn't maintained to keep in sync with the python > version, keeping our python approach stable and continuing to use it makes > breaking the java version (when changes occur in the viewer code) much less > likely. I will attempt to make it work with the current implementation of output_generator.. but there is a chance that it just wont work with clock_syncs/battors with the currently output_generator. If thats the case, someone will have to update the java way. Though, I cant say I'm happy with the fact I can't unify how catapult does it because it because something else is unmaintained... It seems bad design to have to maintain 3 separate ways to generate the trace file.
I echo ccraik@'s sentiment that we should continue using systrace's output_generator for the time being: I don't think that we want a dependency going from systrace to Telemetry, although nednguyen@google.com would know better. Randy, do you know what specifically was broken about the clock syncing with output_generator?
On 2017/02/24 21:14:18, charliea wrote:
> I echo ccraik@'s sentiment that we should continue using systrace's
> output_generator for the time being: I don't think that we want a dependency
> going from systrace to Telemetry, although mailto:nednguyen@google.com would
know
> better.
>
> Randy, do you know what specifically was broken about the clock syncing with
> output_generator?
All I know is the exception that is in the console. I do not really know much
about the trace viewer so it might be better for someone with any knowledge of
it to look at it.
The problem also exists in the current implementation of catapult systrace, even
without any of my changes so there is a very good chance they also exist in the
java version.
Uncaught Error: Expected blinkGcMetric to be in a file named
blink_gc_metric.html; actual: systrace/trace.html; stack: Error
at Function.<anonymous> (trace.html:8786)
at Function.dispatchEvent (trace.html:4156)
at Function.registry.register (trace.html:4182)
at trace.html:8833
at Object.exportTo (trace.html:4141)
at trace.html:8830
at Function.<anonymous> (trace.html:8786)
at Function.dispatchEvent (trace.html:4156)
at Function.registry.register (trace.html:4182)
at trace.html:8833
at Object.exportTo (trace.html:4141)
at trace.html:8830
at Function.<anonymous> (trace.html:8787)
at Function.dispatchEvent (trace.html:4156)
at Function.registry.register (trace.html:4182)
at trace.html:8833
at Object.exportTo (trace.html:4141)
at trace.html:8830
Ok, I made some changes to this so that if it detects only atrace, it will use the old method of creating the trace file. If multiple traces are found it will use the same method telemetry uses. It is useless to compare this to patchset 1 so do not review it that way. https://codereview.chromium.org/2712163002/diff/20001/systrace/systrace/prefi... File systrace/systrace/prefix.html (right): https://codereview.chromium.org/2712163002/diff/20001/systrace/systrace/prefi... systrace/systrace/prefix.html:56: function() { linter complaints.
Description was changed from ========== [Systrace] Fix systrace so that it works. It was broken before. BUG=catapult:#3262 ========== to ========== [Systrace] Fix systrace clock syncing issue with BattOr. It was broken before and they were aligned several hours apart. BUG=catapult:#3262 ==========
The CQ bit was checked by rnephew@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Catapult Mac Tryserver on master.tryserver.client.catapult (JOB_FAILED, https://build.chromium.org/p/tryserver.client.catapult/builders/Catapult%20Ma...)
The CQ bit was checked by rnephew@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Catapult Mac Tryserver on master.tryserver.client.catapult (JOB_FAILED, https://build.chromium.org/p/tryserver.client.catapult/builders/Catapult%20Ma...)
Patchset #4 (id:50001) has been deleted
Patchset #4 (id:60001) has been deleted
The CQ bit was checked by rnephew@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #3 (id:40001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Catapult Linux Tryserver on master.tryserver.client.catapult (JOB_FAILED, https://build.chromium.org/p/tryserver.client.catapult/builders/Catapult%20Li...)
Has BattOr synchronization never worked in the past? This is a large change, which feels like quite a hack - it makes me trust our testing coverage significantly less due to the increase in complexity, and splitting of two completely different trace generation codepaths. I'm all for using telemetry's trace generation code, if appropriate, but I want to make sure we see if we can: 1) work with the current code/structure until we don't need to support the legacy java tracer in the Android SDK 2) move TraceDataBuilder out of telemetry, since that dependency makes systrace go from 13MiB to 27MiB
On 2017/03/01 00:16:16, Chris Craik wrote: > Has BattOr synchronization never worked in the past? It works in telemetry, I dont think it ever worked properly for systrace. > > This is a large change, which feels like quite a hack - it makes me trust our > testing coverage significantly less due to the increase in complexity, and > splitting of two completely different trace generation codepaths. For the case where the new code triggers and the trace file is generated using the telemetry way, its actually less complicated for systrace; since all the heavy lifting is done in catapult/tracing/tracing_build/trace2html. Since any changes to how to generate trace files will occur there it also makes it more resilient to future changes such as clock sync. > > I'm all for using telemetry's trace generation code, if appropriate, but I want > to make sure we see if we can: > > 1) work with the current code/structure until we don't need to support the > legacy java tracer in the Android SDK We need to get systrace working so people can interactively do BattOr tracing on android. chrome://inspect does not work properly on android for BattOr tracing. > > 2) move TraceDataBuilder out of telemetry, since that dependency makes systrace > go from 13MiB to 27MiB There is plans to move it from telemetry to its own part of catapult. IIRC there are ways to pull in just parts of the catapult repo, so it would drastically reduce the amount of code that would have to be pulled in since it would only rely on common/ and tracing/.
Patchset #3 (id:70001) has been deleted
The CQ bit was checked by rnephew@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Overall comment: there's a *lot* going on in this CL that makes it hard to review, and it should almost certainly be split into several CLs. https://codereview.chromium.org/2712163002/diff/90001/systrace/systrace/outpu... File systrace/systrace/output_generator.py (right): https://codereview.chromium.org/2712163002/diff/90001/systrace/systrace/outpu... systrace/systrace/output_generator.py:15: from telemetry.timeline import trace_data We should move this to a common location (in a separate CL) before importing it here https://codereview.chromium.org/2712163002/diff/90001/systrace/systrace/outpu... systrace/systrace/output_generator.py:87: html_file.write(' <script class="viewer-data" type="application/text">\n') This should be its own CL Subject: "Fix the class on the script tag that systrace outputs" https://codereview.chromium.org/2712163002/diff/90001/systrace/systrace/test_... File systrace/systrace/test_data/battor_test_data (right): https://codereview.chromium.org/2712163002/diff/90001/systrace/systrace/test_... systrace/systrace/test_data/battor_test_data:1: # BattOr I might append ".txt" to this file. Otherwise, it sounds like it might be an executable https://codereview.chromium.org/2712163002/diff/90001/systrace/systrace/traci... File systrace/systrace/tracing_agents/battor_trace_agent.py (right): https://codereview.chromium.org/2712163002/diff/90001/systrace/systrace/traci... systrace/systrace/tracing_agents/battor_trace_agent.py:30: battor, target, from_file, device_serial_number): It seems like most of the work in this file should be its own CL https://codereview.chromium.org/2712163002/diff/90001/systrace/systrace/traci... File systrace/systrace/tracing_controller.py (right): https://codereview.chromium.org/2712163002/diff/90001/systrace/systrace/traci... systrace/systrace/tracing_controller.py:82: 'clock-domain': 'TELEMETRY', We almost certainly don't want to be have systrace use a clock domain of TELEMETRY. How about some way of making it use SYSTRACE?
https://codereview.chromium.org/2712163002/diff/90001/systrace/systrace/outpu... File systrace/systrace/output_generator.py (right): https://codereview.chromium.org/2712163002/diff/90001/systrace/systrace/outpu... systrace/systrace/output_generator.py:15: from telemetry.timeline import trace_data On 2017/03/03 21:49:19, charliea wrote: > We should move this to a common location (in a separate CL) before importing it > here Done. https://codereview.chromium.org/2712163002/diff/90001/systrace/systrace/outpu... systrace/systrace/output_generator.py:87: html_file.write(' <script class="viewer-data" type="application/text">\n') On 2017/03/03 21:49:19, charliea wrote: > This should be its own CL > > Subject: "Fix the class on the script tag that systrace outputs" Backing out changes and will make a seperate CL for them. https://codereview.chromium.org/2712163002/diff/90001/systrace/systrace/test_... File systrace/systrace/test_data/battor_test_data (right): https://codereview.chromium.org/2712163002/diff/90001/systrace/systrace/test_... systrace/systrace/test_data/battor_test_data:1: # BattOr On 2017/03/03 21:49:19, charliea wrote: > I might append ".txt" to this file. Otherwise, it sounds like it might be an > executable Done. https://codereview.chromium.org/2712163002/diff/90001/systrace/systrace/traci... File systrace/systrace/tracing_agents/battor_trace_agent.py (right): https://codereview.chromium.org/2712163002/diff/90001/systrace/systrace/traci... systrace/systrace/tracing_agents/battor_trace_agent.py:30: battor, target, from_file, device_serial_number): On 2017/03/03 21:49:20, charliea wrote: > It seems like most of the work in this file should be its own CL I will get another CL out for this shortly, and will rebase this against it. https://codereview.chromium.org/2712163002/diff/90001/systrace/systrace/traci... File systrace/systrace/tracing_controller.py (right): https://codereview.chromium.org/2712163002/diff/90001/systrace/systrace/traci... systrace/systrace/tracing_controller.py:82: 'clock-domain': 'TELEMETRY', On 2017/03/03 21:49:20, charliea wrote: > We almost certainly don't want to be have systrace use a clock domain of > TELEMETRY. How about some way of making it use SYSTRACE? Done.
As I mentioned for the previous CL, I'd really like to avoid taking any dependencies on telemetry. It's 5MB added to the Android SDK, which is an extra 5MB per SDK each Android app developer has installed.
On 2017/03/09 18:41:45, Chris Craik wrote: > As I mentioned for the previous CL, I'd really like to avoid taking any > dependencies on telemetry. It's 5MB added to the Android SDK, which is an extra > 5MB per SDK each Android app developer has installed. Totally agree that systrace must not depend on Telemetry. Not only because of SDK issue, but also because Telemetry already depends on Systrace. Circular dependencies between projects should be avoided.
Could you please add more detail to the description about why this fixes the clock sync between systrace and BattOr? The reason is that, by explicitly specifying the clock domain in the trace metadata (by using the same output formatter as Telemetry), the trace importer uses that clock domain for the systrace controller trace rather than the default (UNKNOWN_CHROME_LEGACY). This means that it avoid collapsing the systrace controller trace (from the host) and LINUX_CLOCK_MONOTONIC (on the device) into a single clock domain, which it shouldn't because they're on different devices.
https://codereview.chromium.org/2712163002/diff/110001/systrace/systrace/outp... File systrace/systrace/output_generator.py (right): https://codereview.chromium.org/2712163002/diff/110001/systrace/systrace/outp... systrace/systrace/output_generator.py:52: # The old method does not support anything except atrace. Shouldn't the new way work equally well if we only have an atrace? https://codereview.chromium.org/2712163002/diff/110001/systrace/systrace/outp... File systrace/systrace/output_generator_unittest.py (right): https://codereview.chromium.org/2712163002/diff/110001/systrace/systrace/outp... systrace/systrace/output_generator_unittest.py:118: def del_if_exist(path): http://stackoverflow.com/a/10840586 suggests that a more pythonic way to do this would be: try: os.remove(path) except OSError: pass Otherwise, there's technically no guarantee that just because a path exists when you check the first line of code that it will also exist by the time you call os.remove() https://codereview.chromium.org/2712163002/diff/110001/systrace/systrace/pref... File systrace/systrace/prefix.html (right): https://codereview.chromium.org/2712163002/diff/110001/systrace/systrace/pref... systrace/systrace/prefix.html:56: function() { Should be able to use arrow functions here: () => { timelineViewEl.model = m; ... }, (err) => { var overlay = new tr.ui.b.Overlay(); ... }); https://codereview.chromium.org/2712163002/diff/110001/systrace/systrace/pref... systrace/systrace/prefix.html:67: } nit: can just do "});" on this line https://codereview.chromium.org/2712163002/diff/110001/systrace/systrace/trac... File systrace/systrace/tracing_agents/battor_trace_agent.py (right): https://codereview.chromium.org/2712163002/diff/110001/systrace/systrace/trac... systrace/systrace/tracing_agents/battor_trace_agent.py:29: def __init__(self, battor_categories, serial_map, battor_path, This was all done in another CL, right? If so, please set the diffbase to that CL with `git branch --set-upstream-to=your_diffbase_branch` and reupload the CL https://codereview.chromium.org/2712163002/diff/110001/tracing/tracing/model/... File tracing/tracing/model/clock_sync_manager.html (right): https://codereview.chromium.org/2712163002/diff/110001/tracing/tracing/model/... tracing/tracing/model/clock_sync_manager.html:28: // "Telemetry" and "Systrace aren't really clock domains because they nit: add " after Systrace
https://codereview.chromium.org/2712163002/diff/110001/systrace/systrace/outp... File systrace/systrace/output_generator.py (right): https://codereview.chromium.org/2712163002/diff/110001/systrace/systrace/outp... systrace/systrace/output_generator.py:52: # The old method does not support anything except atrace. On 2017/03/10 16:18:48, charliea wrote: > Shouldn't the new way work equally well if we only have an atrace? It does, but ccraik@ wanted to maintain the old way as well until the java side of things completely gets rid of it. https://codereview.chromium.org/2712163002/diff/110001/systrace/systrace/outp... File systrace/systrace/output_generator_unittest.py (right): https://codereview.chromium.org/2712163002/diff/110001/systrace/systrace/outp... systrace/systrace/output_generator_unittest.py:118: def del_if_exist(path): On 2017/03/10 16:18:48, charliea wrote: > http://stackoverflow.com/a/10840586 suggests that a more pythonic way to do this > would be: > > try: > os.remove(path) > except OSError: > pass > > Otherwise, there's technically no guarantee that just because a path exists when > you check the first line of code that it will also exist by the time you call > os.remove() Done. https://codereview.chromium.org/2712163002/diff/110001/systrace/systrace/pref... File systrace/systrace/prefix.html (right): https://codereview.chromium.org/2712163002/diff/110001/systrace/systrace/pref... systrace/systrace/prefix.html:56: function() { On 2017/03/10 16:18:48, charliea wrote: > Should be able to use arrow functions here: > > () => { > timelineViewEl.model = m; > ... > }, > (err) => { > var overlay = new tr.ui.b.Overlay(); > ... > }); I didn't write any of this, and only edited the spacing because of a linter error from when I was changing the from viewer-data to trace-data. Since I'm doing that in a seperate CL now, I am oging to back out all changes to this file. I will do the spacing and the arrow function changes in the CL htat I change from viewer to trace. https://codereview.chromium.org/2712163002/diff/110001/systrace/systrace/pref... systrace/systrace/prefix.html:67: } On 2017/03/10 16:18:48, charliea wrote: > nit: can just do "});" on this line Acknowledged. https://codereview.chromium.org/2712163002/diff/110001/systrace/systrace/trac... File systrace/systrace/tracing_agents/battor_trace_agent.py (right): https://codereview.chromium.org/2712163002/diff/110001/systrace/systrace/trac... systrace/systrace/tracing_agents/battor_trace_agent.py:29: def __init__(self, battor_categories, serial_map, battor_path, On 2017/03/10 16:18:48, charliea wrote: > This was all done in another CL, right? If so, please set the diffbase to that > CL with `git branch --set-upstream-to=your_diffbase_branch` and reupload the CL I tried, but when I do that I keep getting git cl upload Using 50% similarity for rename/copy detection. Override with --similarity. ERROR: Your diff contains 17 commits already in refs/remotes/origin/master. Run "git log --oneline 5366cb026ba8bca3a035199265d6d122189e88e9..HEAD" to get a list of commits in the diff. If you are using a custom git flow, you can override the reference used for this check with "git config gitcl.remotebranch <git-ref>" Wouldn't landing the other CL and rebasing this on that yeild the same results though? https://codereview.chromium.org/2712163002/diff/110001/tracing/tracing/model/... File tracing/tracing/model/clock_sync_manager.html (right): https://codereview.chromium.org/2712163002/diff/110001/tracing/tracing/model/... tracing/tracing/model/clock_sync_manager.html:28: // "Telemetry" and "Systrace aren't really clock domains because they On 2017/03/10 16:18:48, charliea wrote: > nit: add " after Systrace Done.
Description was changed from ========== [Systrace] Fix systrace clock syncing issue with BattOr. It was broken before and they were aligned several hours apart. BUG=catapult:#3262 ========== to ========== [Systrace] Fix systrace clock syncing issue with BattOr. It was broken before and they were aligned several hours apart. By explicitly specifying the clock domain in the trace metadata (by using the same output formatter as Telemetry), the trace importer uses that clock domain for the systrace controller trace rather than the default (UNKNOWN_CHROME_LEGACY). This means that it avoids collapsing the systrace controller trace (from the host) and LINUX_CLOCK_MONOTONIC (on the device) into a single clock domain. Since the clocks are on different devices, this would result in misaligned traces. BUG=catapult:#3262 ==========
Rebasing should yield the same effect if the CL is submitted, so I'd just take that route.
On 2017/03/10 20:03:34, charliea wrote: > Rebasing should yield the same effect if the CL is submitted, so I'd just take > that route. Rebased to https://codereview.chromium.org/2736123003/
https://codereview.chromium.org/2712163002/diff/170001/systrace/systrace/outp... File systrace/systrace/output_generator.py (right): https://codereview.chromium.org/2712163002/diff/170001/systrace/systrace/outp... systrace/systrace/output_generator.py:52: # The old method does not support anything except atrace. ccraik@chromium.org, now that the output generator is in tracing/ and not telemetry/, I'd like to reevaluate moving to a world where we _only_ use the trace data builder. My rationale is that I've worked in a codebase before where we had lots of: if detect_new_way(): # Do new way... return # Do old way... and it's a nightmare to keep a mental map of the logical branches in the long run. This seems like a clear case where we don't need to do that and can just kill the last 40 LoC in this function because they're equivalent. What's the benefit of continuing to use the old way of generating traces here? More specifically, what assurances does it give us about things not breaking systrace that wouldn't be better given by just adding a tracing unit test to make sure that the old systrace trace format continues to import successfully? https://codereview.chromium.org/2712163002/diff/170001/systrace/systrace/test... File systrace/systrace/test_data/battor_test_data.txt (right): https://codereview.chromium.org/2712163002/diff/170001/systrace/systrace/test... systrace/systrace/test_data/battor_test_data.txt:1: # BattOr nit: given that this is for a unit test, just keep like ten lines of the trace. no reason to store more than is necessary to make a point that this is a battor test data file. if you do store 500 lines, you run the risk of someone thinking there's a reason why you need to. https://codereview.chromium.org/2712163002/diff/170001/tracing/tracing/model/... File tracing/tracing/model/clock_sync_manager.html (right): https://codereview.chromium.org/2712163002/diff/170001/tracing/tracing/model/... tracing/tracing/model/clock_sync_manager.html:28: // "Telemetry" and "Systrace" aren't really clock domains because they s/"Telemetry"/'TELEMETRY", same for "Systrace"
https://codereview.chromium.org/2712163002/diff/170001/systrace/systrace/outp... File systrace/systrace/output_generator.py (right): https://codereview.chromium.org/2712163002/diff/170001/systrace/systrace/outp... systrace/systrace/output_generator.py:52: # The old method does not support anything except atrace. I agree it's gross, but I really don't want to break java systrace capture in the monitor tool while we still have to support it. I'm reaching out to android tools team to figure out when in the release process it's reasonable to drop it. I'm still not clear though on what moving to the benefit of the new format immediately is. I totally agree that we shouldn't have our own output generator code in the long term, but I don't understand why time sync wouldn't be working in the old system. The problem with testing importing is that it doesn't actually run any of the JS prefix code, which is the part I worry about breaking. We've had many times in the past that the prefix has to be updated to point to changed method or class names for importing the trace change. Continuing to use the old way of generating traces ensures that if prefix.html is broken, output is broken for both entry points the same way. As to unit testing, I don't know of a way of validating that a given generated html file not only opens, but correctly displays a trace. If there's some way to do that, I'd be happy to add that, and enable us to switch over entirely. Only thought I have is that one could get pretty far at checking prefix.html by parsing/loading it in a js test and exec()-ing it, but that seems pretty gross, and I don't think we want exec() in our codebase :P
https://codereview.chromium.org/2712163002/diff/170001/systrace/systrace/outp... File systrace/systrace/output_generator.py (right): https://codereview.chromium.org/2712163002/diff/170001/systrace/systrace/outp... systrace/systrace/output_generator.py:38: trace_data_builder.AsData().Serialize(output_file_name, 'Systrace') 'Systrace' should probably be a constant https://codereview.chromium.org/2712163002/diff/170001/systrace/systrace/outp... systrace/systrace/output_generator.py:52: # The old method does not support anything except atrace. On 2017/03/20 17:38:37, Chris Craik wrote: > I agree it's gross, but I really don't want to break java systrace capture in > the monitor tool while we still have to support it. I'm reaching out to android > tools team to figure out when in the release process it's reasonable to drop it. > > I'm still not clear though on what moving to the benefit of the new format > immediately is. I totally agree that we shouldn't have our own output generator > code in the long term, but I don't understand why time sync wouldn't be working > in the old system. > > The problem with testing importing is that it doesn't actually run any of the JS > prefix code, which is the part I worry about breaking. We've had many times in > the past that the prefix has to be updated to point to changed method or class > names for importing the trace change. Continuing to use the old way of > generating traces ensures that if prefix.html is broken, output is broken for > both entry points the same way. > > As to unit testing, I don't know of a way of validating that a given generated > html file not only opens, but correctly displays a trace. If there's some way to > do that, I'd be happy to add that, and enable us to switch over entirely. > > Only thought I have is that one could get pretty far at checking prefix.html by > parsing/loading it in a js test and exec()-ing it, but that seems pretty gross, > and I don't think we want exec() in our codebase :P Decision based on VC: we'll maintain the dual pathways for the short amount of time that we expect Java systrace to continue existing, then pare down to just the Python output formatter. https://codereview.chromium.org/2712163002/diff/170001/systrace/systrace/outp... systrace/systrace/output_generator.py:52: # The old method does not support anything except atrace. Randy, could you add a TODO describing why we have two pathways when one seems like it should work? https://codereview.chromium.org/2712163002/diff/170001/systrace/systrace/outp... File systrace/systrace/output_generator_unittest.py (right): https://codereview.chromium.org/2712163002/diff/170001/systrace/systrace/outp... systrace/systrace/output_generator_unittest.py:100: # Generate controller trace. nit: I think "Generate controller trace" is implied by .append(...systraceController, {}); https://codereview.chromium.org/2712163002/diff/170001/systrace/systrace/trac... File systrace/systrace/tracing_controller.py (right): https://codereview.chromium.org/2712163002/diff/170001/systrace/systrace/trac... systrace/systrace/tracing_controller.py:79: formatted_data = { This seems like it's probably a hack, and that the tracing output formatter should take care of this, right? I think documenting this with a TODO is justified. https://codereview.chromium.org/2712163002/diff/170001/tracing/tracing/model/... File tracing/tracing/model/clock_sync_manager.html (right): https://codereview.chromium.org/2712163002/diff/170001/tracing/tracing/model/... tracing/tracing/model/clock_sync_manager.html:28: // "Telemetry" and "Systrace" aren't really clock domains because they On 2017/03/20 14:37:38, charliea wrote: > s/"Telemetry"/'TELEMETRY", same for "Systrace" P.S., wasn't this in the patch you already submitted? Something weird might be happening with the diffbases, but I'm not quite sure how to fix.
Couple minor nits, otherwise LGTM https://codereview.chromium.org/2712163002/diff/170001/systrace/systrace/outp... File systrace/systrace/output_generator.py (right): https://codereview.chromium.org/2712163002/diff/170001/systrace/systrace/outp... systrace/systrace/output_generator.py:53: if len(trace_results) > 2: The comment above makes it sound like we should be checking for atrace only here, in addition to length < 2. Should we be, or do we know in advance that only atrace traces will have single entry here? https://codereview.chromium.org/2712163002/diff/170001/systrace/systrace/outp... File systrace/systrace/output_generator_unittest.py (left): https://codereview.chromium.org/2712163002/diff/170001/systrace/systrace/outp... systrace/systrace/output_generator_unittest.py:76: json_data = open(COMBINED_PROFILE_CHROME_DATA).read() can we delete the file COMBINED_PROFILE_CHROME_DATA?
The CQ bit was checked by rnephew@chromium.org to run a CQ dry run
https://codereview.chromium.org/2712163002/diff/170001/systrace/systrace/outp... File systrace/systrace/output_generator.py (right): https://codereview.chromium.org/2712163002/diff/170001/systrace/systrace/outp... systrace/systrace/output_generator.py:38: trace_data_builder.AsData().Serialize(output_file_name, 'Systrace') On 2017/03/21 19:47:23, charliea wrote: > 'Systrace' should probably be a constant Done. https://codereview.chromium.org/2712163002/diff/170001/systrace/systrace/outp... systrace/systrace/output_generator.py:52: # The old method does not support anything except atrace. On 2017/03/21 19:47:23, charliea wrote: > On 2017/03/20 17:38:37, Chris Craik wrote: > > I agree it's gross, but I really don't want to break java systrace capture in > > the monitor tool while we still have to support it. I'm reaching out to > android > > tools team to figure out when in the release process it's reasonable to drop > it. > > > > I'm still not clear though on what moving to the benefit of the new format > > immediately is. I totally agree that we shouldn't have our own output > generator > > code in the long term, but I don't understand why time sync wouldn't be > working > > in the old system. > > > > The problem with testing importing is that it doesn't actually run any of the > JS > > prefix code, which is the part I worry about breaking. We've had many times in > > the past that the prefix has to be updated to point to changed method or class > > names for importing the trace change. Continuing to use the old way of > > generating traces ensures that if prefix.html is broken, output is broken for > > both entry points the same way. > > > > As to unit testing, I don't know of a way of validating that a given generated > > html file not only opens, but correctly displays a trace. If there's some way > to > > do that, I'd be happy to add that, and enable us to switch over entirely. > > > > Only thought I have is that one could get pretty far at checking prefix.html > by > > parsing/loading it in a js test and exec()-ing it, but that seems pretty > gross, > > and I don't think we want exec() in our codebase :P > > Decision based on VC: we'll maintain the dual pathways for the short amount of > time that we expect Java systrace to continue existing, then pare down to just > the Python output formatter. Added comment about removing once java no longer uses the old way. https://codereview.chromium.org/2712163002/diff/170001/systrace/systrace/outp... systrace/systrace/output_generator.py:53: if len(trace_results) > 2: On 2017/03/21 19:52:23, Chris Craik wrote: > The comment above makes it sound like we should be checking for atrace only > here, in addition to length < 2. Should we be, or do we know in advance that > only atrace traces will have single entry here? It should work with any single trace. I reworded it to reflect that reality. https://codereview.chromium.org/2712163002/diff/170001/systrace/systrace/outp... File systrace/systrace/output_generator_unittest.py (left): https://codereview.chromium.org/2712163002/diff/170001/systrace/systrace/outp... systrace/systrace/output_generator_unittest.py:76: json_data = open(COMBINED_PROFILE_CHROME_DATA).read() On 2017/03/21 19:52:23, Chris Craik wrote: > can we delete the file COMBINED_PROFILE_CHROME_DATA? The new unittest still uses it. https://codereview.chromium.org/2712163002/diff/170001/systrace/systrace/outp... File systrace/systrace/output_generator_unittest.py (right): https://codereview.chromium.org/2712163002/diff/170001/systrace/systrace/outp... systrace/systrace/output_generator_unittest.py:100: # Generate controller trace. On 2017/03/21 19:47:23, charliea wrote: > nit: I think "Generate controller trace" is implied by > > .append(...systraceController, {}); Done. https://codereview.chromium.org/2712163002/diff/170001/systrace/systrace/test... File systrace/systrace/test_data/battor_test_data.txt (right): https://codereview.chromium.org/2712163002/diff/170001/systrace/systrace/test... systrace/systrace/test_data/battor_test_data.txt:1: # BattOr On 2017/03/20 14:37:38, charliea wrote: > nit: given that this is for a unit test, just keep like ten lines of the trace. > no reason to store more than is necessary to make a point that this is a battor > test data file. > > if you do store 500 lines, you run the risk of someone thinking there's a reason > why you need to. Done. https://codereview.chromium.org/2712163002/diff/170001/systrace/systrace/trac... File systrace/systrace/tracing_controller.py (right): https://codereview.chromium.org/2712163002/diff/170001/systrace/systrace/trac... systrace/systrace/tracing_controller.py:79: formatted_data = { On 2017/03/21 19:47:23, charliea wrote: > This seems like it's probably a hack, and that the tracing output formatter > should take care of this, right? I think documenting this with a TODO is > justified. Done. https://codereview.chromium.org/2712163002/diff/170001/tracing/tracing/model/... File tracing/tracing/model/clock_sync_manager.html (right): https://codereview.chromium.org/2712163002/diff/170001/tracing/tracing/model/... tracing/tracing/model/clock_sync_manager.html:28: // "Telemetry" and "Systrace" aren't really clock domains because they On 2017/03/21 19:47:23, charliea wrote: > On 2017/03/20 14:37:38, charliea wrote: > > s/"Telemetry"/'TELEMETRY", same for "Systrace" > > P.S., wasn't this in the patch you already submitted? Something weird might be > happening with the diffbases, but I'm not quite sure how to fix. No this was not part of the BattOr tracing agent CL I submitted before. https://codereview.chromium.org/2736123003/ https://codereview.chromium.org/2712163002/diff/170001/tracing/tracing/model/... tracing/tracing/model/clock_sync_manager.html:28: // "Telemetry" and "Systrace" aren't really clock domains because they On 2017/03/20 14:37:38, charliea wrote: > s/"Telemetry"/'TELEMETRY", same for "Systrace" Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
lgtm w/ comment Thanks for fixing this Randy! https://codereview.chromium.org/2712163002/diff/190001/systrace/systrace/outp... File systrace/systrace/output_generator.py (right): https://codereview.chromium.org/2712163002/diff/190001/systrace/systrace/outp... systrace/systrace/output_generator.py:54: # TODO(rnephew): Remove old legacy code when java systrace no longer mirrors nit: given the depth of the conversations that we had, I think a more descriptive comment is probably warranted here. Something like: TODO(rnephew): The tracing output formatter is able to handle a single trace just as well as it handles multiple traces. The obvious thing to do here would be to use it for all systrace output: however, we want to continue using the legacy way of formatting systrace output when a single trace is present in order to match the Java version of systrace. Java systrace should be deleted by <date - ask Chris?>, at which point we should consolidate this logic. The reason I feel like that's important is that your existing comment doesn't really answer the question about why the Python version of systrace is tied to the Java version of systrace at all.
benm@chromium.org changed reviewers: + benm@chromium.org
https://codereview.chromium.org/2712163002/diff/190001/systrace/systrace/outp... File systrace/systrace/output_generator.py (right): https://codereview.chromium.org/2712163002/diff/190001/systrace/systrace/outp... systrace/systrace/output_generator.py:56: if len(trace_results) > 2: Drive by with minimal context (sorry)...: given the comment about multiple traces, should this be >= 2?
https://codereview.chromium.org/2712163002/diff/190001/systrace/systrace/outp... File systrace/systrace/output_generator.py (right): https://codereview.chromium.org/2712163002/diff/190001/systrace/systrace/outp... systrace/systrace/output_generator.py:54: # TODO(rnephew): Remove old legacy code when java systrace no longer mirrors On 2017/03/23 18:32:32, charliea wrote: > nit: given the depth of the conversations that we had, I think a more > descriptive comment is probably warranted here. > > Something like: > > TODO(rnephew): The tracing output formatter is able to handle a single trace > just as well as it handles multiple traces. The obvious thing to do here would > be to use it for all systrace output: however, we want to continue using the > legacy way of formatting systrace output when a single trace is present in order > to match the Java version of systrace. Java systrace should be deleted by <date > - ask Chris?>, at which point we should consolidate this logic. > > The reason I feel like that's important is that your existing comment doesn't > really answer the question about why the Python version of systrace is tied to > the Java version of systrace at all. Done. https://codereview.chromium.org/2712163002/diff/190001/systrace/systrace/outp... systrace/systrace/output_generator.py:56: if len(trace_results) > 2: On 2017/03/28 13:29:33, benm (inactive) wrote: > Drive by with minimal context (sorry)...: given the comment about multiple > traces, should this be >= 2? The 'systrace' tracing agent is ignored in the old code path. I will make that more clear with a comment. https://codereview.chromium.org/2712163002/diff/210001/tracing/tracing/model/... File tracing/tracing/model/clock_sync_manager.html (right): https://codereview.chromium.org/2712163002/diff/210001/tracing/tracing/model/... tracing/tracing/model/clock_sync_manager.html:122: if (markers[0].domainId === domainId) { Changes in this file are from rebasing.
The CQ bit was checked by rnephew@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by rnephew@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from ccraik@google.com, charliea@chromium.org Link to the patchset: https://codereview.chromium.org/2712163002/#ps210001 (title: "rebase and change todo")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 210001, "attempt_start_ts": 1490900921670530,
"parent_rev": "94d1bb9f4ab0a7fbbabe5018ffa0578b054fa89c", "commit_rev":
"790242054d40097c6af06bf42804f73e998eb60c"}
Message was sent while issue was closed.
Description was changed from ========== [Systrace] Fix systrace clock syncing issue with BattOr. It was broken before and they were aligned several hours apart. By explicitly specifying the clock domain in the trace metadata (by using the same output formatter as Telemetry), the trace importer uses that clock domain for the systrace controller trace rather than the default (UNKNOWN_CHROME_LEGACY). This means that it avoids collapsing the systrace controller trace (from the host) and LINUX_CLOCK_MONOTONIC (on the device) into a single clock domain. Since the clocks are on different devices, this would result in misaligned traces. BUG=catapult:#3262 ========== to ========== [Systrace] Fix systrace clock syncing issue with BattOr. It was broken before and they were aligned several hours apart. By explicitly specifying the clock domain in the trace metadata (by using the same output formatter as Telemetry), the trace importer uses that clock domain for the systrace controller trace rather than the default (UNKNOWN_CHROME_LEGACY). This means that it avoids collapsing the systrace controller trace (from the host) and LINUX_CLOCK_MONOTONIC (on the device) into a single clock domain. Since the clocks are on different devices, this would result in misaligned traces. BUG=catapult:#3262 Review-Url: https://codereview.chromium.org/2712163002 Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapu... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:210001) as https://chromium.googlesource.com/external/github.com/catapult-project/catapu... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
