Need a long term solution to simplify the process of rolling systrace into the Android sdk

[nedn]

Currently, catapult/systrace/ depends on many other modules in catapult, those are: catatapult/catapult_base/ catatapult/common/battor catatapult/common/py_trace_event catatapult/devil catatapult/telemetry (wattt?)

This makes it's quite difficult to cleanly roll systrace/ project into Android sdk. I worries that in the long run, more dependencies will sneak in & make it even harder for @ChrisCraik to manually resolve the dependencies.

Potential solutions: Use chromium's isolated testing, i.e: declare a isolate (think of this like a BUILD file) file that specifies all the dependent files of systrace/ project. During test time, the chrome infra will use the isolate to create a zip file of all dependent files, send it to a machine pool, extract the zip & run tests with only those files. This would help: 1) ensuring the dependencies are kept in check, 2) Chris can just take the zip file produced from the isolate testing, then unpack & roll to Android sdk. (Also see https://github.com/catapult-project/catapult/issues/1889)

@ChrisCraik : what would prevent you from rolling all the files in catapult/ to Android sdk? Is it the code size that's problematic?

@Alex319

[ChrisCraik]

Yes, it's storage size. Android SDK/platform tools copy that I have (sdk 6.0 rev 3) is 9mb, with the systrace directory being 1.1mb. Catapult is > 300mb, which would be a huge increase in download size for the vast majority of Android devs that don't use command line systrace.

We could also consider something like a dependency blacklist - third_party/, telemetry/, tracing/, and perf_insights/ make up the vast majority of storage space. If they aren't used by systrace/, that's a lot of easy savings. That might be easier to enforce/maintain, though it doesn't scale well to more top level directories being added.

[Alex319]

As for why systrace depends on these items:

The only dependency of systrace on telemetry is on the function _atexit_withlog in telemetry.internal.util. The _battor_traceagent.py in systrace imports _atexit_withlog in order to register a callback to re-enable charging on the device after systrace exits. If this were not there then the device could get stuck in a non-charging state if systrace exited unexpectedly. (Thus, it may be possible to remove this dependency if necessary.)

Systrace depends on catapult_base because catapult_base is needed by py_trace_event in order to enable tracing. If catapult_base is not in the PYTHONPATH then _trace_event.trace_canenable() will return FALSE, and _trace_event.traceenable() will fail. _traceevent is what records the controller agent clock sync markers.

Of course, systrace depends on common/battor for the BattOr wrapper, it depends on common/py_trace_event for recording the controller agent clock sync markers, and it depends on utils for lots of utility functions including communication via ADB.

-Alexander Mont

On Thu, Apr 21, 2016 at 1:39 PM, Chris Craik notifications@github.com wrote:

Yes, it's storage size. Android SDK/platform tools copy that I have (sdk 6.0 rev 3) is 9mb, with the systrace directory being 1.1mb. Catapult is > 300mb, which would be a huge increase in download size for the vast majority of Android devs that don't use command line systrace.

We could also consider something like a dependency blacklist - third_party/, telemetry/, tracing/, and perf_insights/ make up the vast majority of storage space. If they aren't used by systrace/, that's a lot of easy savings. That might be easier to enforce/maintain, though it doesn't scale well to more top level directories being added.

— You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub https://github.com/catapult-project/catapult/issues/2275#issuecomment-213104863

[nedn]

@Alex319 can you remove the telemetry dependency? I think other deps are ok to keep for now.

[nedn]

Ping @Alex319 can you remove the telemetry dependency from systrace? Otherwise it would cause trouble for @ChrisCraik the next time he rolls systrace/

[wangzhen127]

@Alex319 What's the current status of cleaning up the dependencies?

@peterwashington

[Alex319]

I believe that I got rid of the dependency of systrace on telemetry a long time ago. I just checked and the dependency based on the _atexit_withlog function does not appear to still be there. Are you still experiencing problems related to this dependency?

-Alexander Mont

On Thu, Jul 14, 2016 at 3:28 PM, Zhen Wang notifications@github.com wrote:

@Alex319 https://github.com/Alex319 What's the current status of cleaning up the dependencies?

@peterwashington https://github.com/peterwashington

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/catapult-project/catapult/issues/2275#issuecomment-232811170, or mute the thread https://github.com/notifications/unsubscribe-auth/AOKcFRY2PpJW9A2SpEplKK2tzF0AhOXoks5qVrgjgaJpZM4INCqp .

[wangzhen127]

Nice! We should probably remove the following line and update the comment? https://cs.chromium.org/chromium/src/third_party/catapult/systrace/systrace/__init__.py?q=telemetry&l=29

[ChrisCraik]

I just updated Systrace in AOSP, and manually declared the updated dependencies again: https://android-review.googlesource.com/#/c/261012/1/build/sdk.atree

Jreck@ had a good idea while reviewing - from within catapult, provide a means to build a minimum-dependencies copy of Systrace. Then the update script calls that, and the SDK build just deep copies that version.

We could even run all systrace tests on trybots with that copied version, so it becomes much more difficult for the SDK version of Systrace to break.

[wangzhen127]

I like that auto approach.

@anniesullie @nedn Are other parts of Catapult already have some similar requirements or features?

[nedn]

@ChrisCraik what you described is the isolated testing . For scripts, our build file would just list of a bunch directory to be included in the zip (call the "isolate"). We haven't got time to isolate the catapult tests yet, but it shouldn't be too hard from infra point of view.

@ehanley324

[ChrisCraik]

@nedn I'm happy to poke into this as part of rolling updates to AOSP. What do folks think about me just rolling a manual own version into catapult/systrace/bin/run_tests, as default behavior? (Is there any existing infrastructure I could share?)

If we want to generalize it later to other tests, we can, but that would solve a lot of the pain around pushing to upstream.

[nedn]

@ehanley324 is most up-to-date about how swarming & isolate work. She can help with defining the dependency for systrace & swarming its test suite.

[ehanley324]

So I am currently working on swarming the perf benchmarks, but the concept is similar. Take a look at my design document around creating the isolate.

https://docs.google.com/document/d/12L1AXbkgL9wJMe_a2YfLCBvZwQKmwBJdEUPVaFT7isk/edit

It sounds like you had done a lot of the heavy lifting which is determining what the dependencies are. Once you have a .gn file it is pretty much boilerplate to add an isolate to the gn build. See step 2 in the document above.

If your hardware already exists in the swarming pool it is easy from there to trigger a local task, but if not you need to work with infra to get that setup before you can trigger your job.

[ChrisCraik]

I feel like I'm missing some background information here - apologies, I'm not familiar with the Chrome build system or bot infrastructure.

Is the build infrastructure you describe in your docs used within catapult for python? My plan was to simply add a few lines to our top level testing script to declare/deep-copy dependency directories manually into an output directory where all the tests would run. Then trybots would start testing the isolated version automatically.

It may be that we have entirely different motivations - our only goal for isolation is testing/generating a minimum subset for distribution outside of the catapult repository - not for fitting on constrained testing bots.

All of our tests and infrastructure would be included in the isolated version, and it doesn't have to be perfectly minimal - just skipping the javascript code and test data gets us down to a handful of megabytes. It would actually increase the working set size for the existing trybots, but just by that trivial few megabytes, since that's all that Systrace needs to function downstream in AOSP / the Android SDK.

If we were to use the build infrastructure you mention, would that be easily integrated into our python test infrastructure?

[ChrisCraik]

We're now working around this issue downstream in AOSP - with a new change from John, we now just whitelist the catapult directories to depend upon, and only check in those: https://android-review.googlesource.com/#/c/292263/

Now building the systrace in the SDK is just a matter of copying the entire (checked in portion) of catapult. This means we can test and verify at sync time, when pulling from catapult. Not really ideal, since it means we only catch new/bad dependencies rarely, but folks have been pretty diligent at avoiding accidental new dependencies in systrace. Still much better than catching failures in the SDK after we shipped it publicly, and means we can trust AOSP copy === SDK copy.

We could try and get the dependency checking into continuous testing (maybe by deep copying certain dependencies when running the systrace script), but I think we can punt on that until it becomes a problem.