Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2867253002:
Enable cc_perftests, gpu_perftests, and tracing_perftests on Nexus 5X Perf (Closed)
DescriptionEnable cc_perftests, gpu_perftests, and tracing_perftests on Nexus 5X Perf
BUG=chromium:715565
Review-Url: https://codereview.chromium.org/2867253002
Cr-Commit-Position: refs/heads/master@{#470328}
Committed: https://chromium.googlesource.com/chromium/src/+/6451d7d746ccdb4cf7c322118ac1147ec7f2d35d
Patch Set 1 #
Total comments: 3
Patch Set 2 : Switch to use //third_party/android_tools/sdk/platform-tools/adb #
Messages
Total messages: 27 (14 generated)
nednguyen@google.com changed reviewers: + dpranke@chromium.org, kbr@chromium.org, martiniss@chromium.org
Although these tests weren't enabled on Nexus 5X Perf before (see https://cs.chromium.org/chromium/src/tools/perf/core/perf_data_generator.py?q...), this is done a a test to verify that we can actually schedule these C++ microbenchmarks on swarmed Nexus 5X device before we move forward with migrating other Nexus perf bots to swarming.
The CQ bit was checked by nednguyen@google.com to run a CQ dry run
https://codereview.chromium.org/2867253002/diff/1/tools/perf/core/perf_data_g... File tools/perf/core/perf_data_generator.py (right): https://codereview.chromium.org/2867253002/diff/1/tools/perf/core/perf_data_g... tools/perf/core/perf_data_generator.py:264: ('tracing_perftests', 'build73-b1--device2'), The 'build73-b1--device2' bot is picked because it's the least loaded one.
lgtm
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Enable cc_perftests, gpu_perftests, and tracing_perftests on Nexus 5X Perf BUG=chromium:715565 ========== to ========== Enable cc_perftests, gpu_perftests, and tracing_perftests on Nexus 5X Perf BUG=chromium:715565 ==========
nednguyen@google.com changed reviewers: + jbudorick@chromium.org
https://codereview.chromium.org/2867253002/diff/1/testing/buildbot/gn_isolate... File testing/buildbot/gn_isolate_map.pyl (right): https://codereview.chromium.org/2867253002/diff/1/testing/buildbot/gn_isolate... testing/buildbot/gn_isolate_map.pyl:945: "src/third_party/catapult/devil/bin/deps/linux2/x86_64/bin/adb" John: I am copying this from https://cs.chromium.org/chromium/src/testing/buildbot/gn_isolate_map.pyl?rcl=... However, I am suspecting that Emily made a mistake back then. How does src/third_party/catapult/devil/bin/deps/linux2/x86_64/bin/adb even be generated on the client? Unless there is some script that tries to download the deps file of devil in gclient sync.
On 2017/05/09 00:08:17, nednguyen wrote: > https://codereview.chromium.org/2867253002/diff/1/testing/buildbot/gn_isolate... > File testing/buildbot/gn_isolate_map.pyl (right): > > https://codereview.chromium.org/2867253002/diff/1/testing/buildbot/gn_isolate... > testing/buildbot/gn_isolate_map.pyl:945: > "src/third_party/catapult/devil/bin/deps/linux2/x86_64/bin/adb" > John: I am copying this from > https://cs.chromium.org/chromium/src/testing/buildbot/gn_isolate_map.pyl?rcl=... > > > However, I am suspecting that Emily made a mistake back then. How does > src/third_party/catapult/devil/bin/deps/linux2/x86_64/bin/adb even be generated > on the client? Unless there is some script that tries to download the deps file > of devil in gclient sync. I am switching to using third_party/android_tools/sdk/platform-tools/adb to be sure. PTAL
nednguyen@google.com changed reviewers: - kbr@chromium.org
The CQ bit was checked by nednguyen@google.com 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.
lgtm https://codereview.chromium.org/2867253002/diff/1/testing/buildbot/gn_isolate... File testing/buildbot/gn_isolate_map.pyl (right): https://codereview.chromium.org/2867253002/diff/1/testing/buildbot/gn_isolate... testing/buildbot/gn_isolate_map.pyl:945: "src/third_party/catapult/devil/bin/deps/linux2/x86_64/bin/adb" On 2017/05/09 00:08:17, nednguyen wrote: > John: I am copying this from > https://cs.chromium.org/chromium/src/testing/buildbot/gn_isolate_map.pyl?rcl=... > > > However, I am suspecting that Emily made a mistake back then. How does > src/third_party/catapult/devil/bin/deps/linux2/x86_64/bin/adb even be generated > on the client? Unless there is some script that tries to download the deps file > of devil in gclient sync. FWIW, I think Emily was correct here. Using the chromium adb rather than devil adb should be OK on swarming, but it'd likely break the unswarmed bots because telemetry uses devil's adb & we'd have a version mismatch.
On 2017/05/09 14:10:12, jbudorick wrote: > lgtm > > https://codereview.chromium.org/2867253002/diff/1/testing/buildbot/gn_isolate... > File testing/buildbot/gn_isolate_map.pyl (right): > > https://codereview.chromium.org/2867253002/diff/1/testing/buildbot/gn_isolate... > testing/buildbot/gn_isolate_map.pyl:945: > "src/third_party/catapult/devil/bin/deps/linux2/x86_64/bin/adb" > On 2017/05/09 00:08:17, nednguyen wrote: > > John: I am copying this from > > > https://cs.chromium.org/chromium/src/testing/buildbot/gn_isolate_map.pyl?rcl=... > > > > > > However, I am suspecting that Emily made a mistake back then. How does > > src/third_party/catapult/devil/bin/deps/linux2/x86_64/bin/adb even be > generated > > on the client? Unless there is some script that tries to download the deps > file > > of devil in gclient sync. > > FWIW, I think Emily was correct here. Using the chromium adb rather than devil > adb should be OK on swarming, but it'd likely break the unswarmed bots because > telemetry uses devil's adb & we'd have a version mismatch. Sorry, what I meant was not about which adb version to use, but about I could not find the code for fetching devil adb from cloud storage during steps of creating the isolate. So I suspect this was some mistake.
The CQ bit was checked by nednguyen@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from martiniss@chromium.org Link to the patchset: https://codereview.chromium.org/2867253002/#ps20001 (title: "Switch to use //third_party/android_tools/sdk/platform-tools/adb")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/05/09 14:27:59, nednguyen wrote: > On 2017/05/09 14:10:12, jbudorick wrote: > > lgtm > > > > > https://codereview.chromium.org/2867253002/diff/1/testing/buildbot/gn_isolate... > > File testing/buildbot/gn_isolate_map.pyl (right): > > > > > https://codereview.chromium.org/2867253002/diff/1/testing/buildbot/gn_isolate... > > testing/buildbot/gn_isolate_map.pyl:945: > > "src/third_party/catapult/devil/bin/deps/linux2/x86_64/bin/adb" > > On 2017/05/09 00:08:17, nednguyen wrote: > > > John: I am copying this from > > > > > > https://cs.chromium.org/chromium/src/testing/buildbot/gn_isolate_map.pyl?rcl=... > > > > > > > > > However, I am suspecting that Emily made a mistake back then. How does > > > src/third_party/catapult/devil/bin/deps/linux2/x86_64/bin/adb even be > > generated > > > on the client? Unless there is some script that tries to download the deps > > file > > > of devil in gclient sync. > > > > FWIW, I think Emily was correct here. Using the chromium adb rather than devil > > adb should be OK on swarming, but it'd likely break the unswarmed bots because > > telemetry uses devil's adb & we'd have a version mismatch. > > Sorry, what I meant was not about which adb version to use, but about I could > not find the code for fetching devil adb from cloud storage during steps of > creating the isolate. So I suspect this was some mistake. Ah, I misunderstood you, then, but I actually don't think that's a mistake, either. You're correct that we don't fetch devil adb during isolate creation, but it's accounted for in the isolate via the dependencies JSON file.
CQ is committing da patch.
Bot data: {"patchset_id": 20001, "attempt_start_ts": 1494340082994450,
"parent_rev": "0168e580be3bd318a42f7de3b55e39379b0dcdd8", "commit_rev":
"6451d7d746ccdb4cf7c322118ac1147ec7f2d35d"}
Message was sent while issue was closed.
Description was changed from ========== Enable cc_perftests, gpu_perftests, and tracing_perftests on Nexus 5X Perf BUG=chromium:715565 ========== to ========== Enable cc_perftests, gpu_perftests, and tracing_perftests on Nexus 5X Perf BUG=chromium:715565 Review-Url: https://codereview.chromium.org/2867253002 Cr-Commit-Position: refs/heads/master@{#470328} Committed: https://chromium.googlesource.com/chromium/src/+/6451d7d746ccdb4cf7c322118ac1... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/6451d7d746ccdb4cf7c322118ac1...
Message was sent while issue was closed.
On 2017/05/09 14:30:53, jbudorick wrote: > On 2017/05/09 14:27:59, nednguyen wrote: > > On 2017/05/09 14:10:12, jbudorick wrote: > > > lgtm > > > > > > > > > https://codereview.chromium.org/2867253002/diff/1/testing/buildbot/gn_isolate... > > > File testing/buildbot/gn_isolate_map.pyl (right): > > > > > > > > > https://codereview.chromium.org/2867253002/diff/1/testing/buildbot/gn_isolate... > > > testing/buildbot/gn_isolate_map.pyl:945: > > > "src/third_party/catapult/devil/bin/deps/linux2/x86_64/bin/adb" > > > On 2017/05/09 00:08:17, nednguyen wrote: > > > > John: I am copying this from > > > > > > > > > > https://cs.chromium.org/chromium/src/testing/buildbot/gn_isolate_map.pyl?rcl=... > > > > > > > > > > > > However, I am suspecting that Emily made a mistake back then. How does > > > > src/third_party/catapult/devil/bin/deps/linux2/x86_64/bin/adb even be > > > generated > > > > on the client? Unless there is some script that tries to download the deps > > > file > > > > of devil in gclient sync. > > > > > > FWIW, I think Emily was correct here. Using the chromium adb rather than > devil > > > adb should be OK on swarming, but it'd likely break the unswarmed bots > because > > > telemetry uses devil's adb & we'd have a version mismatch. > > > > Sorry, what I meant was not about which adb version to use, but about I could > > not find the code for fetching devil adb from cloud storage during steps of > > creating the isolate. So I suspect this was some mistake. > > Ah, I misunderstood you, then, but I actually don't think that's a mistake, > either. You're correct that we don't fetch devil adb during isolate creation, > but it's accounted for in the isolate via the dependencies JSON file. Does "testing/scripts/run_gtest_perf_test.py gpu_perftests ..." reuse devil code? Otherwise how do they know to use dependency manager infra to get the dep in dependencies JSON file?
Message was sent while issue was closed.
On 2017/05/09 14:39:17, nednguyen wrote: > On 2017/05/09 14:30:53, jbudorick wrote: > > On 2017/05/09 14:27:59, nednguyen wrote: > > > On 2017/05/09 14:10:12, jbudorick wrote: > > > > lgtm > > > > > > > > > > > > > > https://codereview.chromium.org/2867253002/diff/1/testing/buildbot/gn_isolate... > > > > File testing/buildbot/gn_isolate_map.pyl (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2867253002/diff/1/testing/buildbot/gn_isolate... > > > > testing/buildbot/gn_isolate_map.pyl:945: > > > > "src/third_party/catapult/devil/bin/deps/linux2/x86_64/bin/adb" > > > > On 2017/05/09 00:08:17, nednguyen wrote: > > > > > John: I am copying this from > > > > > > > > > > > > > > > https://cs.chromium.org/chromium/src/testing/buildbot/gn_isolate_map.pyl?rcl=... > > > > > > > > > > > > > > > However, I am suspecting that Emily made a mistake back then. How does > > > > > src/third_party/catapult/devil/bin/deps/linux2/x86_64/bin/adb even be > > > > generated > > > > > on the client? Unless there is some script that tries to download the > deps > > > > file > > > > > of devil in gclient sync. > > > > > > > > FWIW, I think Emily was correct here. Using the chromium adb rather than > > devil > > > > adb should be OK on swarming, but it'd likely break the unswarmed bots > > because > > > > telemetry uses devil's adb & we'd have a version mismatch. > > > > > > Sorry, what I meant was not about which adb version to use, but about I > could > > > not find the code for fetching devil adb from cloud storage during steps of > > > creating the isolate. So I suspect this was some mistake. > > > > Ah, I misunderstood you, then, but I actually don't think that's a mistake, > > either. You're correct that we don't fetch devil adb during isolate creation, > > but it's accounted for in the isolate via the dependencies JSON file. > > Does "testing/scripts/run_gtest_perf_test.py gpu_perftests ..." reuse devil > code? Otherwise how do they know to use dependency manager infra to get the dep > in dependencies JSON file? gtests run through //build/android/test_runner.py which uses devil.
Message was sent while issue was closed.
On 2017/05/09 16:05:37, jbudorick wrote: > On 2017/05/09 14:39:17, nednguyen wrote: > > On 2017/05/09 14:30:53, jbudorick wrote: > > > On 2017/05/09 14:27:59, nednguyen wrote: > > > > On 2017/05/09 14:10:12, jbudorick wrote: > > > > > lgtm > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2867253002/diff/1/testing/buildbot/gn_isolate... > > > > > File testing/buildbot/gn_isolate_map.pyl (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2867253002/diff/1/testing/buildbot/gn_isolate... > > > > > testing/buildbot/gn_isolate_map.pyl:945: > > > > > "src/third_party/catapult/devil/bin/deps/linux2/x86_64/bin/adb" > > > > > On 2017/05/09 00:08:17, nednguyen wrote: > > > > > > John: I am copying this from > > > > > > > > > > > > > > > > > > > > > https://cs.chromium.org/chromium/src/testing/buildbot/gn_isolate_map.pyl?rcl=... > > > > > > > > > > > > > > > > > > However, I am suspecting that Emily made a mistake back then. How does > > > > > > src/third_party/catapult/devil/bin/deps/linux2/x86_64/bin/adb even be > > > > > generated > > > > > > on the client? Unless there is some script that tries to download the > > deps > > > > > file > > > > > > of devil in gclient sync. > > > > > > > > > > FWIW, I think Emily was correct here. Using the chromium adb rather than > > > devil > > > > > adb should be OK on swarming, but it'd likely break the unswarmed bots > > > because > > > > > telemetry uses devil's adb & we'd have a version mismatch. > > > > > > > > Sorry, what I meant was not about which adb version to use, but about I > > could > > > > not find the code for fetching devil adb from cloud storage during steps > of > > > > creating the isolate. So I suspect this was some mistake. > > > > > > Ah, I misunderstood you, then, but I actually don't think that's a mistake, > > > either. You're correct that we don't fetch devil adb during isolate > creation, > > > but it's accounted for in the isolate via the dependencies JSON file. > > > > Does "testing/scripts/run_gtest_perf_test.py gpu_perftests ..." reuse devil > > code? Otherwise how do they know to use dependency manager infra to get the > dep > > in dependencies JSON file? > > gtests run through //build/android/test_runner.py which uses devil. Ah got it. I wasn't sure how the magic from C++ calling python lib happens :-). Once I can confirm that this is working, I will switch these back to using devil adb for consistency. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
